Skip to content

Conversation

@PeterDaveHello
Copy link
Member

@PeterDaveHello PeterDaveHello commented Aug 30, 2025

User description

Replace JS/CSS minifiers with esbuild to reduce build times while preserving outputs.

Enable thread-loader by default in dev and production. Choose workers dynamically (based on CPU cores) and allow overrides via environment variables. Keep filesystem cache enabled and make cache compression configurable, defaulting to uncompressed for faster warm builds on CPU-bound machines. Add BUILD_PARALLEL toggle (default on) to switch between parallel and sequential production variant builds. Ensure watch-once dev builds exit cleanly.

Adopt sass-embedded for SASS processing. In development, use style-loader to speed up CSS/SCSS compilation while keeping production outputs unchanged. Maintain CSP-safe source maps for extension contexts and suppress CSS 404 noise in development outputs.

Additionally, dependency caching has been added to the GitHub Actions workflow to accelerate CI/CD runs.

Results on this DO VPS (2 cores, ~4 GiB RAM):

  • Production first run (cold): ~44s (baseline ~105s)
  • Production second run (warm): ~19s (baseline ~39s)
  • Development first run: ~31s; second run: ~29s

Times vary by environment; numbers above are for this machine.

Pull request summary from GitHub Copilot:

This pull request introduces several significant improvements to the build process, focusing on performance, configurability, and developer experience. The main changes include switching to faster build tools, adding advanced caching and parallelization options, updating build scripts, and improving CI build efficiency. These updates should result in faster builds, better resource utilization, and more flexibility for both local development and CI environments.

Build Performance and Tooling Improvements

  • Switched JS minification from Terser to esbuild (via esbuild-loader) and enabled esbuild-based CSS minification for much faster production builds. (build.mjs, package.json) [1] [2]
  • Added support for parallel builds of production variants, controlled via the BUILD_PARALLEL environment variable, and enabled thread-loader for Babel by default (configurable via BUILD_THREAD and BUILD_THREAD_WORKERS). (build.mjs, .github/copilot-instructions.md) [1] [2] [3]
  • Introduced advanced Webpack cache options, allowing filesystem cache compression to be toggled with BUILD_CACHE_COMPRESSION for improved warm build performance. (build.mjs, .github/copilot-instructions.md) [1] [2]

Build Script and Output Enhancements

  • Updated the build script to support both parallel and sequential builds, improved output directory management, and ensured correct copying of build artifacts for both production and development. (build.mjs) [1] [2]
  • In development mode, the script now creates placeholder CSS files to prevent 404 errors in the browser. (build.mjs) [1] [2]

Continuous Integration (CI) Improvements

  • Added npm and Webpack cache steps to the pre-release GitHub Actions workflow to speed up CI builds. (.github/workflows/pre-release-build.yml)

Dependency Updates

  • Added new dependencies: esbuild, esbuild-loader, sass-embedded, style-loader, and thread-loader; updated sass and sass-loader to latest versions; removed terser-webpack-plugin as it's no longer used. (package.json) [1] [2]

Documentation

  • Expanded the build instructions to document all new performance-related environment variables and their effects. (.github/copilot-instructions.md)

PR Type

Enhancement


Description

  • Replace Terser/CSS minifiers with esbuild for faster builds

  • Enable thread-loader by default with configurable parallelism

  • Add filesystem cache compression control and parallel build options

  • Switch to sass-embedded and style-loader for dev builds

  • Add GitHub Actions dependency caching for CI acceleration


Diagram Walkthrough

flowchart LR
  A["Old Build System"] --> B["esbuild Minification"]
  A --> C["Thread-loader Parallelism"]
  A --> D["Advanced Webpack Caching"]
  A --> E["sass-embedded Processing"]
  B --> F["Faster Production Builds"]
  C --> F
  D --> F
  E --> F
  G["GitHub Actions"] --> H["Dependency Caching"]
  H --> I["Faster CI Builds"]
Loading

File Walkthrough

Relevant files
Enhancement
build.mjs
Major build system optimization with esbuild and threading

build.mjs

  • Replace Terser with esbuild for JS/CSS minification
  • Add thread-loader with configurable worker count
  • Implement filesystem cache compression control
  • Enable parallel/sequential build modes via BUILD_PARALLEL
  • Switch to sass-embedded and conditional style-loader usage
  • Add watch-once mode for development builds
+167/-51
pre-release-build.yml
Add dependency and build caching to CI workflow                   

.github/workflows/pre-release-build.yml

  • Add npm cache using actions/cache@v4
  • Add Webpack filesystem cache for build acceleration
  • Cache both ~/.npm and .cache/webpack directories
+16/-0   
Dependencies
package.json
Update dependencies for build optimization                             

package.json

  • Add esbuild and esbuild-loader dependencies
  • Replace terser-webpack-plugin with esbuild-loader
  • Update sass to sass-embedded with newer sass-loader
  • Add style-loader and thread-loader dependencies
+7/-3     
Documentation
copilot-instructions.md
Document new build performance configuration options         

.github/copilot-instructions.md

  • Document new build performance environment variables
  • Explain BUILD_PARALLEL, BUILD_THREAD, BUILD_CACHE_COMPRESSION options
  • Add BUILD_WATCH_ONCE documentation for development
+16/-0   

Summary by CodeRabbit

  • Chores

    • Improved CI caching for Node/npm and Webpack filesystem; exports Node version for builds.
    • Updated build dependencies (esbuild tooling, new Sass runtime, thread-loader, style-loader, babel runtime).
  • Refactor

    • Per-variant build caching and artifact isolation, optional parallel/threaded builds, streaming packaging, dev-only CSS/sourcemap placeholders, and stronger watch/error handling.
    • Switched to esbuild-based JS/CSS minification and updated development vs production CSS handling.
  • Documentation

    • Added Build Performance Options covering build flags, defaults, and multi-variant output.

✏️ Tip: You can customize this high-level summary in your review settings.

@PeterDaveHello PeterDaveHello requested a review from Copilot August 30, 2025 20:15
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 30, 2025

📝 Walkthrough

Walkthrough

Adds CI npm and Webpack filesystem caching; refactors build.mjs into a variant-aware, optionally parallel build system with per-variant filesystem cache, esbuild-based minification, optional thread-loader, improved asset copying/zipping, sourceBuildDir support, and updates docs and dependencies.

Changes

Cohort / File(s) Summary
CI workflow caching
.github/workflows/pre-release-build.yml
Enable actions/setup-node npm cache and export NODE_VERSION; add actions/cache@v4 steps to cache Webpack filesystem cache (paths: .cache/webpack, node_modules/.cache/webpack) keyed by OS, NODE_VERSION, webpack and lockfile/build hash; build steps unchanged.
Build system core overhaul
build.mjs
Add env helpers; compute per-variant variantId/dirKey and support sourceBuildDir; enable per-variant filesystem cache with optional compression; switch to esbuild-based JS/CSS minification; add optional thread-loader; select Sass implementation (sass-embedded fallback); switch CSS loaders by env; rewrite zipFolder/copyFiles to streaming archiver with dev placeholders; orchestrate per-variant parallel/sequential builds and robust watch/close/error handling.
Dependencies and tooling
package.json
Add @babel/runtime, esbuild, esbuild-loader, style-loader, thread-loader; add sass-embedded; upgrade sass/sass-loader; remove terser-webpack-plugin.
Documentation updates
AGENTS.md
Document build performance flags (BUILD_PARALLEL, BUILD_THREAD, BUILD_THREAD_WORKERS, BUILD_CACHE_COMPRESSION, BUILD_WATCH_ONCE, BUILD_POOL_TIMEOUT, BUILD_RESOLVE_SYMLINKS), default behavior (esbuild minification, CSS handling), and multi-variant production outputs.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer / CI
  participant Build as build.mjs
  participant WP as Webpack Compiler
  participant FS as Filesystem
  participant Arch as Archiver/Zip
  Note over Build,WP: Per-variant orchestration, caching & esbuild minification

  Dev->>Build: invoke build (env flags, sourceBuildDir)
  activate Build
  Build->>Build: compute variants, dirKey, variantId, cache keys
  alt Parallel builds
    par Variant A
      Build->>WP: runWebpack(variant A, sourceBuildDir)
      WP-->>FS: emit assets to per-variant dir (uses filesystem cache)
      Build->>FS: copyFiles & finishOutput(variant A)
      Build->>Arch: zipFolder(variant A)
    and Variant B
      Build->>WP: runWebpack(variant B, sourceBuildDir)
      WP-->>FS: emit assets to per-variant dir
      Build->>FS: copyFiles & finishOutput(variant B)
      Build->>Arch: zipFolder(variant B)
    end
  else Sequential builds
    loop each variant
      Build->>WP: runWebpack(variant, sourceBuildDir)
      WP-->>FS: emit assets
      Build->>FS: finishOutput
      Build->>Arch: zipFolder
    end
  end
  Build-->>Dev: exit status (0 / non-zero on fatal)
  deactivate Build
Loading
sequenceDiagram
  autonumber
  actor CI as GitHub Actions
  participant Setup as actions/setup-node
  participant CacheNpm as npm-cache (setup-node)
  participant CacheWP as actions/cache
  participant Job as build job

  CI->>Setup: setup-node (cache: npm)
  Setup-->>CacheNpm: restore npm cache (OS + lockfile)
  CI->>Job: export NODE_VERSION
  CI->>CacheWP: restore .cache/webpack using OS + NODE_VERSION + lockfile/build hash
  CI->>Job: npm ci && npm run build
  Job-->>CacheWP: save updated Webpack filesystem cache
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

dependencies, github_actions

Poem

I thump my paws—cache warms the trail,
Threads hum softly, builds set sail.
Esbuild breezes, Sass in tune,
Variants spin beneath the moon.
Zip! goes the bundle—swift and light. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main refactoring focus: improving build pipeline performance and dev output reliability, which aligns with the substantial changes across build.mjs, CI workflow, and dependencies.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 65e8ca6 and 88bc3bc.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • .github/workflows/pre-release-build.yml
  • AGENTS.md
  • build.mjs
  • package.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • .github/workflows/pre-release-build.yml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:165-171
Timestamp: 2025-09-14T18:01:17.933Z
Learning: In the ChatGPTBox project, the build.mjs webpack cache buildDependencies configuration should only track npm lockfiles (package-lock.json) and not include other package managers' lockfiles like pnpm-lock.yaml, yarn.lock, or bun.lockb, as confirmed by the maintainer PeterDaveHello.
📚 Learning: 2025-09-14T18:01:17.933Z
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:165-171
Timestamp: 2025-09-14T18:01:17.933Z
Learning: In the ChatGPTBox project, the build.mjs webpack cache buildDependencies configuration should only track npm lockfiles (package-lock.json) and not include other package managers' lockfiles like pnpm-lock.yaml, yarn.lock, or bun.lockb, as confirmed by the maintainer PeterDaveHello.

Applied to files:

  • build.mjs
📚 Learning: 2025-09-06T17:46:04.304Z
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.304Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.

Applied to files:

  • build.mjs
🔇 Additional comments (12)
AGENTS.md (1)

20-39: Documentation is accurate and well-organized.

The new "Build Performance Options" section correctly documents all the environment variables and their behaviors as implemented in build.mjs. The note on line 31 clarifying that Babel loader cache compression is independent of BUILD_CACHE_COMPRESSION is helpful for avoiding confusion.

build.mjs (11)

17-49: Well-implemented environment helpers with proper validation.

The getBooleanEnv and parseCacheCompressionOption functions provide clear, consistent parsing with helpful warnings for invalid values. The try-catch around os.cpus() adds robustness for edge cases.


50-84: Thread worker parsing and feature toggles are well-structured.

The parseThreadWorkerCount function properly validates input using a strict regex pattern, caps to CPU count, and provides clear warnings. The BUILD_POOL_TIMEOUT parsing includes proper validation.


86-107: Sass implementation caching is properly implemented.

The getSassImplementation function correctly caches the Sass implementation promise, avoiding repeated dynamic imports across build variants. Both sass-embedded and sass fallback use consistent mod.default || mod handling, and errors are properly logged.


130-181: Cache configuration is well-designed with per-variant naming.

The variant ID uses a Windows-safe __ separator, and the cache configuration properly tracks build dependencies. The package-lock.json-only approach aligns with the project's npm-based workflow. Based on learnings, this is the intended configuration.


182-194: Esbuild-based minification is correctly configured.

The switch from Terser to esbuild improves build performance. The legalComments: 'none' setting is acceptable for this project's distribution model, as confirmed by the maintainer in previous discussions. Based on learnings, this is the intended behavior.


252-287: Thread-loader and Babel configuration are well-tuned.

The conditional thread-loader setup with variant-aware pool timeouts is appropriate: production uses a brief timeout for cleanup, watch-once uses 0 for immediate exit, and watch uses Infinity to keep workers warm. Babel caching with disabled compression prioritizes build speed.


289-333: CSS loader configuration optimizes for dev speed and production output.

Using style-loader in development for faster CSS injection and MiniCssExtractPlugin.loader in production for extracted files is a good pattern. The quietDeps: true option appropriately suppresses noisy deprecation warnings from third-party dependencies.


406-457: Compiler lifecycle handles errors robustly.

The production path ensures compiler.close() is always called, with proper handling for both synchronous throws and async rejections in the callback. The watch-once mode correctly awaits the callback using Promise.resolve().then() before finalizing, ensuring packaging failures are reflected in the exit code.


460-540: Asset handling is robust with proper dev/prod differentiation.

The zipFolder function uses comprehensive error handling with settled flag to prevent edge cases. The copyFiles function appropriately tolerates missing CSS in development while failing for missing production artifacts. The ensureDevCssPlaceholders function now derives the CSS list dynamically, avoiding hardcoded filenames.


542-605: Output finalization handles both platforms consistently.

The finishOutput function correctly copies source maps only in development for CSP-safe debugging, and dynamically derives CSS placeholder paths from commonFiles to avoid drift. Both Chromium and Firefox outputs are handled consistently.


607-689: Build orchestration handles parallel/sequential builds and errors correctly.

The createWebpackBuildPromise helper properly catches early rejections from runWebpack. Using Promise.allSettled for parallel builds allows both variants to complete before failing, and try/finally ensures temp directories are cleaned up. The watch-once path correctly re-throws packaging failures to ensure non-zero exit.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @PeterDaveHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request comprehensively refactors the build pipeline to enhance performance, configurability, and developer experience. By leveraging modern tools and optimizing existing processes, it aims to drastically reduce build times for both local development and production environments, leading to a more efficient and responsive workflow.

Highlights

  • Build Tooling Upgrade: Replaced JavaScript and CSS minifiers with esbuild for significantly faster build times, improving both development and production workflows.
  • Parallel Processing & Threading: Enabled thread-loader by default for Babel with dynamic worker allocation based on CPU cores, and introduced parallel production variant builds, all configurable via new environment variables.
  • Optimized Caching: Implemented configurable Webpack filesystem cache compression, defaulting to no compression for faster warm builds on CPU-bound machines, enhancing build efficiency.
  • SASS/CSS Enhancements: Adopted sass-embedded for SASS processing and utilized style-loader for faster CSS/SCSS compilation in development, while ensuring production outputs remain unchanged.
  • Development Experience Improvements: Ensured 'watch-once' development builds exit cleanly and added placeholder CSS files to prevent 404 errors and reduce noise during development.
  • CI/CD Efficiency: Integrated dependency caching into GitHub Actions workflows to accelerate continuous integration and deployment runs.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@qodo-code-review
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The new runWebpack signature adds sourceBuildDir before callback, but many internal calls pass parameters that may shift positions incorrectly if any call sites or future edits assume the old order. Validate all invocations use the new 5-arg form consistently and that sourceBuildDir is always a string when expected.

async function runWebpack(isWithoutKatex, isWithoutTiktoken, minimal, sourceBuildDir, callback) {
  const shared = [
    'preact',
    'webextension-polyfill',
    '@primer/octicons-react',
    'react-bootstrap-icons',
    'countries-list',
Watch Exit Logic

In dev mode, compiler.watch closes only when BUILD_WATCH_ONCE is set, but callback still runs after close is requested. Confirm no race or partial close causes unresolved promises or dangling processes in CI. Consider handling watching.close completion before resolving.

if (isProduction) compiler.run(callback)
else {
  const watching = compiler.watch({}, (err, stats) => {
    callback(err, stats)
    if (process.env.BUILD_WATCH_ONCE) watching.close(() => {})
  })
}
Cache Key Specificity

Webpack cache key uses only package-lock.json hash; config/env-driven build flags (e.g., BUILD_CACHE_COMPRESSION, BUILD_THREAD_WORKERS) won’t bust cache. Validate cache correctness across differing CI env vars or include build.mjs hash/env in the cache key.

- name: Cache Webpack filesystem cache
  uses: actions/cache@v4
  with:
    path: |
      .cache/webpack
      node_modules/.cache/webpack
    key: ${{ runner.os }}-webpack-${{ hashFiles('**/package-lock.json') }}
    restore-keys: |
      ${{ runner.os }}-webpack-

This comment was marked as outdated.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request significantly refactors the build pipeline for performance, introducing esbuild for minification, thread-loader for parallelism, and sass-embedded for SASS compilation. The changes are well-structured and bring substantial performance improvements. My review focuses on improving maintainability by reducing code duplication in the build script. I've identified two areas where helper functions could be introduced to make the code cleaner and more reusable: one for creating placeholder CSS files and another for handling the webpack build process.

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Aug 30, 2025

PR Code Suggestions ✨

Latest suggestions up to 5663e3c

CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure production failures exit non-zero

In production, if the callback throws or returns a rejected promise, the process
may succeed silently because only process.exitCode is set on close errors. Set
process.exitCode = 1 when stats contain errors or when the callback fails,
ensuring CI fails reliably. Mirror the watch-once failure behavior.

build.mjs [386-430]

 if (isProduction) {
   // Ensure compiler is properly closed after production runs
   compiler.run((err, stats) => {
+    const hasErrors = !!(
+      err ||
+      (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
+    )
+    let callbackFailed = false
     const finishClose = () =>
       compiler.close((closeErr) => {
         if (closeErr) {
           console.error('Error closing compiler:', closeErr)
           process.exitCode = 1
         }
+        if (hasErrors || callbackFailed) {
+          process.exitCode = 1
+        }
       })
     try {
       const ret = callback(err, stats)
       if (ret && typeof ret.then === 'function') {
-        ret.then(finishClose, finishClose)
+        ret.then(finishClose, () => {
+          callbackFailed = true
+          finishClose()
+        })
       } else {
         finishClose()
       }
     } catch (_) {
+      callbackFailed = true
       finishClose()
     }
   })
 } else {
   const watching = compiler.watch({}, (err, stats) => {
     const hasErrors = !!(
       err ||
       (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
     )
     // Normalize callback return into a Promise to catch synchronous throws
     const ret = Promise.resolve().then(() => callback(err, stats))
     if (isWatchOnce) {
       const finalize = (callbackFailed = false) =>
         watching.close((closeErr) => {
           if (closeErr) console.error('Error closing watcher:', closeErr)
           // Exit explicitly to prevent hanging processes in CI
-          // Use non-zero exit code when errors occurred, including callback failures
           const shouldFail = hasErrors || closeErr || callbackFailed
           process.exit(shouldFail ? 1 : 0)
         })
       ret.then(
         () => finalize(false),
         () => finalize(true),
       )
     }
   })
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This is a critical bug fix for CI/CD, as it ensures that a failed production build correctly returns a non-zero exit code, preventing broken builds from being treated as successful.

Medium
Fix boolean env parsing

This helper treats any non-empty string as true, so BUILD_WATCH_ONCE="" or an
unknown value becomes truthy, which can unexpectedly exit watchers in CI. Align
its behavior with isDisabledEnv to only enable on explicit truthy tokens. This
prevents accidental activation from empty or malformed env values.

build.mjs [18-22]

 function parseBooleanEnv(val) {
   if (val == null) return false
   const s = String(val).trim().toLowerCase()
-  return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off')
+  if (s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off' || s === 'null' || s === 'undefined') return false
+  if (s === '1' || s === 'true' || s === 'yes' || s === 'on') return true
+  return false
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that parseBooleanEnv is too lenient, which could cause unexpected behavior; the proposed change makes environment variable parsing more robust.

Low
Incremental [*]
Harden cache invalidation keys

Include process.version and the major versions of critical loaders/minifiers in
the cache version to prevent stale cache reuse across Node or plugin upgrades
that may change output semantics

build.mjs [149-165]

-const dirKey = path.basename(sourceBuildDir || outdir)
-const variantParts = [
-  isWithoutKatex ? 'no-katex' : 'with-katex',
-  isWithoutTiktoken ? 'no-tiktoken' : 'with-tiktoken',
-  minimal ? 'minimal' : 'full',
-  dirKey,
-  isProduction ? 'prod' : 'dev',
-]
-const variantId = variantParts.join('__')
+const toolVersions = {
+  node: process.version,
+  webpack: webpack.version,
+  loaders: {
+    babelLoader: (() => {
+      try { return require('babel-loader/package.json').version } catch { return 'unknown' }
+    })(),
+    cssMinimizer: (() => {
+      try { return require('css-minimizer-webpack-plugin/package.json').version } catch { return 'unknown' }
+    })(),
+    esbuild: (() => {
+      try { return require('esbuild/package.json').version } catch { return 'unknown' }
+    })(),
+    esbuildLoader: (() => {
+      try { return require('esbuild-loader/package.json').version } catch { return 'unknown' }
+    })(),
+    miniCssExtract: (() => {
+      try { return require('mini-css-extract-plugin/package.json').version } catch { return 'unknown' }
+    })(),
+  },
+}
 ...
 cache: {
   type: 'filesystem',
   name: `webpack-${variantId}`,
-  // Only include dimensions that affect module outputs to avoid
-  // unnecessary cache invalidations across machines/CI runners
-  version: JSON.stringify({ PROD: isProduction }),
-  // default none; override via BUILD_CACHE_COMPRESSION=gzip|brotli
+  version: JSON.stringify({ PROD: isProduction, tools: toolVersions }),
   compression: cacheCompressionOption ?? false,
   buildDependencies: {
     config: [
       path.resolve('build.mjs'),
       ...['package.json', 'package-lock.json']
         .map((p) => path.resolve(p))
         .filter((p) => fs.existsSync(p)),
     ],
   },
 },

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the Webpack cache key should include versions of key build tools to prevent stale cache issues after dependency upgrades, which is a valuable improvement for cache robustness.

Medium
Stabilize Sass builds and warnings

Pass sassOptions: { silentDeps: true } to avoid excessive rebuilds when Sass
includes from node_modules change timestamps and to reduce noisy warnings,
stabilizing dev watch and CI determinism

build.mjs [284-288]

 {
   loader: 'sass-loader',
-  options: { implementation: sassImpl },
+  options: {
+    implementation: sassImpl,
+    sassOptions: {
+      silentDeps: true,
+    },
+  },
 },
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This suggestion correctly proposes using sassOptions: { silentDeps: true } to suppress warnings from third-party Sass files, which improves the developer experience by reducing log noise during builds.

Low
General
Validate worker count strictly

Number.isInteger(raw) is always true for parseInt, but parseInt('1e2', 10)
returns 1 and hides invalid inputs. Reject non-decimal numeric formats to avoid
surprising caps or overuse. Add a strict regex check so only plain positive
integers are accepted.

build.mjs [48-65]

-const cacheCompressionOption = parseCacheCompressionOption(process.env.BUILD_CACHE_COMPRESSION)
-let cpuCount = 1
-try {
-  // os.cpus() returns an array in Node.js; guard with try/catch for portability
-  cpuCount = Math.max(1, os.cpus().length || 1)
-} catch {
-  cpuCount = 1
-}
 function parseThreadWorkerCount(envValue, cpuCount) {
   const maxWorkers = Math.max(1, cpuCount)
   if (envValue !== undefined && envValue !== null) {
-    const raw = parseInt(envValue, 10)
-    if (Number.isInteger(raw) && raw > 0) {
+    const str = String(envValue).trim()
+    if (/^[1-9]\d*$/.test(str)) {
+      const raw = Number(str)
       if (raw > maxWorkers) {
-        console.warn(
-          `[build] BUILD_THREAD_WORKERS=${raw} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`,
-        )
+        console.warn(`[build] BUILD_THREAD_WORKERS=${raw} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`)
       }
       return Math.min(raw, maxWorkers)
     }
-    console.warn(
-      `[build] Invalid BUILD_THREAD_WORKERS="${envValue}", defaulting to CPU count (${cpuCount})`,
-    )
+    console.warn(`[build] Invalid BUILD_THREAD_WORKERS="${envValue}", defaulting to CPU count (${cpuCount})`)
   }
   return maxWorkers
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a weakness in using parseInt for validation and provides a more robust regex-based solution, preventing unexpected behavior from malformed environment variables.

Low
  • Update

Previous suggestions

✅ Suggestions up to commit 6c89d29
CategorySuggestion                                                                                                                                    Impact
General
Prevent unnecessary cache busting
Suggestion Impact:The commit removed THREAD and WORKERS (and other fields) from the cache version and limited it to { PROD: isProduction }, aligning with the suggestion to avoid unstable values in the version.

code diff:

-      version: JSON.stringify({
-        THREAD: enableThread,
-        WORKERS: threadWorkers,
-        CACHE_COMP: cacheCompressionOption ?? false,
-        PROD: isProduction,
-      }),
+      // Only include dimensions that affect module outputs to avoid
+      // unnecessary cache invalidations across machines/CI runners
+      version: JSON.stringify({ PROD: isProduction }),

Avoid putting unstable values (like worker count) into the cache version, or it
will constantly invalidate caches across machines/CI runners. Limit the cache
version to configuration that truly affects module outputs.

build.mjs [138-157]

 const compiler = webpack({
   ...
   devtool: isProduction ? false : 'cheap-module-source-map',
   cache: {
     type: 'filesystem',
     name: `webpack-${variantId}`,
     version: JSON.stringify({
       THREAD: enableThread,
-      WORKERS: threadWorkers,
       CACHE_COMP: cacheCompressionOption ?? false,
       PROD: isProduction,
     }),
-    // default none; override via BUILD_CACHE_COMPRESSION=gzip|brotli
     compression: cacheCompressionOption ?? false,
     buildDependencies: {
       config: [
         path.resolve('build.mjs'),
         ...['package.json', 'package-lock.json']
           .map((p) => path.resolve(p))
           .filter((p) => fs.existsSync(p)),
       ],
     },
   },
   ...
 })
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that including the threadWorkers count, which depends on the machine's CPU cores, in the cache version will reduce cache hits across different environments like local development and CI.

Medium
Incremental [*]
Make symlink resolution configurable

Add a conditional to re-enable resolve.symlinks when an explicit environment
flag is set for linked/workspace development to avoid module duplication and
resolution bugs when using npm link/pnpm during local development.

build.mjs [204-211]

 resolve: {
   extensions: ['.jsx', '.mjs', '.js'],
-  // Disable symlink resolution for consistent behavior/perf; note this can
-  // affect `npm link`/pnpm workspaces during local development
-  symlinks: false,
+  // Disable symlink resolution by default; allow opt-in for linked/workspace dev
+  symlinks: process.env.BUILD_RESOLVE_SYMLINKS === '1',
   alias: {
     parse5: path.resolve(__dirname, 'node_modules/parse5'),
     ...(minimal
Suggestion importance[1-10]: 6

__

Why: The suggestion improves developer experience by adding a configurable flag for symlinks, which the PR's own comments identify as a potential issue for local development with linked packages.

Low
Possible issue
Fix env flag parsing
Suggestion Impact:The commit added a parseBooleanEnv function and used it to compute isWatchOnce, normalizing values like "0", "false", "no", "off", and empty string to false, addressing the fragile truthiness check.

code diff:

+function parseBooleanEnv(val) {
+  if (val == null) return false
+  const s = String(val).trim().toLowerCase()
+  return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off')
+}
+const isWatchOnce = parseBooleanEnv(process.env.BUILD_WATCH_ONCE)

Parse boolean-like env flags explicitly; the current truthiness check treats any
non-empty string (including "0" or "false") as true. This can unintentionally
enable watch-once mode. Normalize values like "0", "false", and "" to false.

build.mjs [16-18]

 const isAnalyzing = process.argv[2] === '--analyze'
 const parallelBuild = process.env.BUILD_PARALLEL !== '0'
-const isWatchOnce = !!process.env.BUILD_WATCH_ONCE
+const isWatchOnce = (() => {
+  const v = process.env.BUILD_WATCH_ONCE
+  if (v == null) return false
+  const s = String(v).trim().toLowerCase()
+  return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off')
+})()

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that !!process.env.BUILD_WATCH_ONCE is a fragile way to parse a boolean environment variable, leading to incorrect behavior if a user sets it to "0" or "false".

Low
✅ Suggestions up to commit abfc14d
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Use external source maps in dev

Align devtool with external map files by switching to external source maps in
development to avoid large inlined maps and ensure the .map files actually exist
for copying.

build.mjs [137-522]

-devtool: isProduction ? false : 'cheap-module-source-map',
+devtool: isProduction ? false : 'source-map',
 ...
-// Dev-only: copy external source maps for CSP-safe debugging
 ...(isProduction
   ? []
   : [
       { src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' },
       { src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
       { src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
       { src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
       { src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
     ]),
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that cheap-module-source-map inlines source maps, which contradicts the goal of copying external .map files for CSP-safe debugging in development.

Medium
Possible issue
Resolve zip on finish or close
Suggestion Impact:The commit added handling for the 'close' event and refactored resolution via a shared done function, ensuring the Promise resolves when the stream closes. It also modified the 'finish' handler to trigger a close, addressing the robustness concern.

code diff:

+    const done = () => {
+      if (!settled) {
+        settled = true
+        resolve()
+      }
+    }
     output.once('error', fail)
     archive.once('error', fail)
     archive.on('warning', (err) => {
       // Log non-fatal archive warnings for diagnostics
       console.warn('[build][zip] warning:', err)
     })
+    // Resolve on close to ensure FD is flushed and closed
+    output.once('close', done)
+    // Ensure close is emitted after finish on some fast runners
     output.once('finish', () => {
-      if (!settled) {
-        settled = true
-        resolve()
+      try {
+        if (typeof output.close === 'function') output.close()
+      } catch (_) {
+        // ignore
       }
     })

If archiver emits 'close' without 'finish' in some Node streams, the promise may
never resolve. Listen to both 'finish' and 'close' on the output stream to
guarantee completion across environments, avoiding stuck builds in CI.

build.mjs [425-452]

 const zipPath = `${dir}.zip`
 await fs.ensureDir(path.dirname(zipPath))
 await new Promise((resolve, reject) => {
   const output = fs.createWriteStream(zipPath)
   const archive = archiver('zip', { zlib: { level: 9 } })
   let settled = false
+  const settleOk = () => {
+    if (!settled) {
+      settled = true
+      resolve()
+    }
+  }
   const fail = (err) => {
     if (!settled) {
       settled = true
       reject(err)
     }
   }
   output.once('error', fail)
+  output.once('finish', settleOk)
+  output.once('close', settleOk)
   archive.once('error', fail)
   archive.on('warning', (err) => {
-    // Log non-fatal archive warnings for diagnostics
     console.warn('[build][zip] warning:', err)
-  })
-  output.once('finish', () => {
-    if (!settled) {
-      settled = true
-      resolve()
-    }
   })
   archive.pipe(output)
   archive.directory(dir, false)
   archive.finalize()
 })
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion for improving robustness. While the finish event on a write stream is generally sufficient, adding a handler for close provides a fallback to prevent hung builds in edge cases, which is valuable for CI stability.

Medium
General
Reduce default thread oversubscription

Spawning threads equal to total CPUs can oversubscribe when Webpack already
parallelizes minifiers and loaders, causing slowdowns or OOM in CI. Cap default
workers to CPU-1 (min 1) unless explicitly overridden to reduce contention.

build.mjs [55]

+function parseThreadWorkerCount(envValue, cpuCount) {
+  const hardMax = Math.max(1, cpuCount - 1) // leave one core for the main thread/system
+  if (envValue !== undefined && envValue !== null) {
+    const raw = parseInt(envValue, 10)
+    if (Number.isInteger(raw) && raw > 0) {
+      if (raw > cpuCount) {
+        console.warn(
+          `[build] BUILD_THREAD_WORKERS=${raw} exceeds CPU count (${cpuCount}); capping to ${cpuCount}`,
+        )
+      }
+      return Math.min(raw, cpuCount)
+    }
+    console.warn(
+      `[build] Invalid BUILD_THREAD_WORKERS="${envValue}", defaulting to ${hardMax}`,
+    )
+  }
+  return hardMax
+}
 const threadWorkers = parseThreadWorkerCount(process.env.BUILD_THREAD_WORKERS, cpuCount)
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential performance issue with thread oversubscription and proposes a reasonable heuristic (CPU-1) to mitigate it, improving build performance on multi-core machines without explicit configuration.

Low
✅ Suggestions up to commit b5f27c3
CategorySuggestion                                                                                                                                    Impact
General
Wait for stream close for ZIP
Suggestion Impact:The commit added resolving on the 'close' event and adjusted the 'finish' handler to trigger close, ensuring the promise resolves after the stream is closed, aligning with the suggestion's intent.

code diff:

+    const done = () => {
+      if (!settled) {
+        settled = true
+        resolve()
+      }
+    }
     output.once('error', fail)
     archive.once('error', fail)
     archive.on('warning', (err) => {
       // Log non-fatal archive warnings for diagnostics
       console.warn('[build][zip] warning:', err)
     })
+    // Resolve on close to ensure FD is flushed and closed
+    output.once('close', done)
+    // Ensure close is emitted after finish on some fast runners
     output.once('finish', () => {
-      if (!settled) {
-        settled = true
-        resolve()
+      try {
+        if (typeof output.close === 'function') output.close()
+      } catch (_) {
+        // ignore
       }
     })

Listen for the close event on the output stream to ensure the file descriptor is
flushed and closed before proceeding. This avoids intermittent truncated ZIPs on
fast CI runners.

build.mjs [431-459]

 async function zipFolder(dir) {
   const zipPath = `${dir}.zip`
   await fs.ensureDir(path.dirname(zipPath))
   await new Promise((resolve, reject) => {
     const output = fs.createWriteStream(zipPath)
     const archive = archiver('zip', { zlib: { level: 9 } })
     let settled = false
-    const fail = (err) => {
+    const settle = (err) => {
       if (!settled) {
         settled = true
-        reject(err)
+        err ? reject(err) : resolve()
       }
     }
+    const fail = (err) => settle(err)
     output.once('error', fail)
     archive.once('error', fail)
     archive.on('warning', (err) => {
-      // Log non-fatal archive warnings for diagnostics
       console.warn('[build][zip] warning:', err)
     })
-    output.once('finish', () => {
-      if (!settled) {
-        settled = true
-        resolve()
-      }
-    })
+    // finish fires when all data is flushed to the OS, but close ensures fd closed
+    output.once('close', () => settle())
     archive.pipe(output)
     archive.directory(dir, false)
     archive.finalize()
   })
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential race condition where the promise could resolve before the file stream is fully closed, and proposes listening to the close event, which is a critical fix for ensuring ZIP file integrity.

Medium
Possible issue
Cap worker pool below CPU count

Cap worker count to at least 1 and at most CPU count minus one to avoid starving
the main thread or oversubscribing small machines. This prevents build hangs or
thrashing on low-core CI runners.

build.mjs [43-62]

 const rawWorkers = process.env.BUILD_THREAD_WORKERS
   ? parseInt(process.env.BUILD_THREAD_WORKERS, 10)
   : undefined
 let threadWorkers
+const maxUsable = Math.max(1, cpuCount - 1) // keep one core for main thread/OS
 if (Number.isInteger(rawWorkers) && rawWorkers > 0) {
-  const maxWorkers = Math.max(1, cpuCount)
-  if (rawWorkers > maxWorkers) {
+  if (rawWorkers > maxUsable) {
     console.warn(
-      `[build] BUILD_THREAD_WORKERS=${rawWorkers} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`,
+      `[build] BUILD_THREAD_WORKERS=${rawWorkers} exceeds usable cores (${maxUsable}); capping to ${maxUsable}`,
     )
   }
-  threadWorkers = Math.min(rawWorkers, maxWorkers)
+  threadWorkers = Math.min(rawWorkers, maxUsable)
 } else {
   if (process.env.BUILD_THREAD_WORKERS) {
     console.warn(
-      `[build] Invalid BUILD_THREAD_WORKERS="${process.env.BUILD_THREAD_WORKERS}", defaulting to CPU count (${cpuCount})`,
+      `[build] Invalid BUILD_THREAD_WORKERS="${process.env.BUILD_THREAD_WORKERS}", defaulting to ${maxUsable}`,
     )
   }
-  threadWorkers = cpuCount
+  threadWorkers = maxUsable
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential performance issue where using all CPU cores for workers can starve the main thread, and proposes a standard practice of reserving one core, which improves build stability.

Medium
✅ Suggestions up to commit 0fdeae8
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Fix CI cache ordering

Move the Webpack cache restore step before installing dependencies so that cache
hits are not invalidated by files created during npm ci, and add cache: 'npm' to
actions/setup-node to ensure package caching is reliable and keyed to the
lockfile and Node version

.github/workflows/pre-release-build.yml [24-46]

+- uses: actions/checkout@v5
 - uses: actions/setup-node@v4
   with:
     node-version: 20
+    cache: 'npm'
+    cache-dependency-path: '**/package-lock.json'
 - name: Detect Node major version
   run: echo "NODE_MAJOR=$(node -p 'process.versions.node.split(".")[0]')" >> $GITHUB_ENV
-- name: Cache npm cache
-  uses: actions/cache@v4
-  with:
-    path: ~/.npm
-    key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
-    restore-keys: |
-      ${{ runner.os }}-node-
-- run: npm ci
 - name: Cache Webpack filesystem cache
   uses: actions/cache@v4
   with:
     path: |
       .cache/webpack
       node_modules/.cache/webpack
     key: ${{ runner.os }}-node${{ env.NODE_MAJOR }}-webpack-${{ hashFiles('**/package-lock.json') }}
     restore-keys: |
       ${{ runner.os }}-node${{ env.NODE_MAJOR }}-webpack-
+- run: npm ci
 - run: npm run build
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using the built-in cache: 'npm' for the setup-node action is best practice and that the Webpack cache should be restored before npm ci, improving the CI workflow's reliability and correctness.

Medium
Avoid copy failures for missing assets
Suggestion Impact:While it didn’t implement the exact file-existence filter before copying, the commit addressed the same problem by adding ensureDevCssPlaceholders to create placeholder CSS and sourcemap files in development, preventing copy failures and 404s due to missing optional assets.

code diff:

+// In development, create placeholder CSS and sourcemap files to avoid 404 noise
+async function ensureDevCssPlaceholders(targetDir) {
+  if (isProduction) return
+  const cssFiles = [path.join(targetDir, 'popup.css'), path.join(targetDir, 'content-script.css')]
+  for (const cssPath of cssFiles) {
+    if (!(await fs.pathExists(cssPath))) {
+      await fs.outputFile(cssPath, '/* dev placeholder */\n')
+    }
+    const mapPath = `${cssPath}.map`
+    if (!(await fs.pathExists(mapPath))) {
+      await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
+    }
+  }
+}
+
 async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) {
   const commonFiles = [
     { src: 'src/logo.png', dst: 'logo.png' },
@@ -520,22 +528,7 @@
     [...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
     chromiumOutputDir,
   )
-  // In development, ensure placeholder CSS and CSS sourcemap files exist to avoid 404 noise
-  if (!isProduction) {
-    const chromiumCssPlaceholders = [
-      path.join(chromiumOutputDir, 'popup.css'),
-      path.join(chromiumOutputDir, 'content-script.css'),
-    ]
-    for (const p of chromiumCssPlaceholders) {
-      if (!(await fs.pathExists(p))) {
-        await fs.outputFile(p, '/* dev placeholder */\n')
-      }
-      const mapPath = `${p}.map`
-      if (!(await fs.pathExists(mapPath))) {
-        await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
-      }
-    }
-  }
+  await ensureDevCssPlaceholders(chromiumOutputDir)
   if (isProduction) await zipFolder(chromiumOutputDir)
 
   // firefox
@@ -544,22 +537,7 @@
     [...commonFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }],
     firefoxOutputDir,
   )
-  // In development, ensure placeholder CSS and CSS sourcemap files exist to avoid 404 noise
-  if (!isProduction) {
-    const firefoxCssPlaceholders = [
-      path.join(firefoxOutputDir, 'popup.css'),
-      path.join(firefoxOutputDir, 'content-script.css'),
-    ]
-    for (const p of firefoxCssPlaceholders) {
-      if (!(await fs.pathExists(p))) {
-        await fs.outputFile(p, '/* dev placeholder */\n')
-      }
-      const mapPath = `${p}.map`
-      if (!(await fs.pathExists(mapPath))) {
-        await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
-      }
-    }
-  }
+  await ensureDevCssPlaceholders(firefoxOutputDir)
   if (isProduction) await zipFolder(firefoxOutputDir)
 }

Guard optional assets before copying by filtering commonFiles to entries that
exist to avoid hard failures when a chunk or map is not emitted (e.g.,
tree-shaken CSS or watch-first run), and log a warning instead of throwing for
missing dev maps

build.mjs [468-523]

 async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) {
-  const commonFiles = [
+  const maybeFiles = [
     { src: 'src/logo.png', dst: 'logo.png' },
     { src: 'src/rules.json', dst: 'rules.json' },
-
     { src: `${sourceBuildDir}/shared.js`, dst: 'shared.js' },
-    { src: `${sourceBuildDir}/content-script.css`, dst: 'content-script.css' }, // shared
-
+    { src: `${sourceBuildDir}/content-script.css', dst: 'content-script.css' },
     { src: `${sourceBuildDir}/content-script.js`, dst: 'content-script.js' },
-
     { src: `${sourceBuildDir}/background.js`, dst: 'background.js' },
-
     { src: `${sourceBuildDir}/popup.js`, dst: 'popup.js' },
     { src: `${sourceBuildDir}/popup.css`, dst: 'popup.css' },
     { src: 'src/popup/index.html', dst: 'popup.html' },
-
     { src: `${sourceBuildDir}/IndependentPanel.js`, dst: 'IndependentPanel.js' },
     { src: 'src/pages/IndependentPanel/index.html', dst: 'IndependentPanel.html' },
     ...(isProduction
       ? []
       : [
           { src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' },
           { src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
           { src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
           { src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
           { src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
         ]),
   ]
+  const commonFiles = []
+  for (const f of maybeFiles) {
+    if (await fs.pathExists(f.src)) {
+      commonFiles.push(f)
+    } else if (!isProduction) {
+      console.warn('[build] optional asset missing (dev):', f.src)
+    }
+  }
   ...
   await copyFiles(
     [...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
     chromiumOutputDir,
   )
   ...
   await copyFiles(
     [...commonFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }],
     firefoxOutputDir,
   )
+}
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the PR's implementation in copyFiles would cause the build to fail if an optional development file like a source map is missing, and proposes a more robust solution by checking for file existence first.

Low
Possible issue
Fail clearly when Sass missing
Suggestion Impact:The commit added a nested try/catch: it first tries to import 'sass-embedded', then falls back to 'sass', and if both fail, it throws a clear error message indicating no Sass implementation is available.

code diff:

+  } catch (e1) {
+    try {
+      const mod = await import('sass')
+      sassImpl = mod.default || mod
+    } catch (e2) {
+      throw new Error(`No Sass implementation available. Install 'sass-embedded' or 'sass'.`)
+    }

Guard against absence of both Sass implementations to prevent runtime crashes.
Throw a clear error message if neither module can be resolved so failures are
actionable.

build.mjs [97-104]

-const sassImpl
+let sassImpl
 try {
   const mod = await import('sass-embedded')
   sassImpl = mod.default || mod
-} catch (e) {
-  const mod = await import('sass')
-  sassImpl = mod.default || mod
+} catch (_) {
+  try {
+    const mod = await import('sass')
+    sassImpl = mod.default || mod
+  } catch (err) {
+    throw new Error(
+      `No Sass implementation available. Install 'sass-embedded' or 'sass'. Original error: ${err && err.message}`,
+    )
+  }
 }
Suggestion importance[1-10]: 7

__

Why: This suggestion improves the build script's robustness by adding explicit error handling for when neither sass-embedded nor sass is available, providing a clear and actionable error message.

Medium
General
Leave one CPU core free

Cap the default worker count to CPU count minus one to avoid exhausting all
cores, which can cause system thrashing in CI and on laptops. Preserve explicit
overrides, but when not set, prefer leaving one core free.

build.mjs [43-62]

 const rawWorkers = process.env.BUILD_THREAD_WORKERS
   ? parseInt(process.env.BUILD_THREAD_WORKERS, 10)
   : undefined
 let threadWorkers
 if (Number.isInteger(rawWorkers) && rawWorkers > 0) {
   const maxWorkers = Math.max(1, cpuCount)
   if (rawWorkers > maxWorkers) {
     console.warn(
       `[build] BUILD_THREAD_WORKERS=${rawWorkers} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`,
     )
   }
   threadWorkers = Math.min(rawWorkers, maxWorkers)
 } else {
   if (process.env.BUILD_THREAD_WORKERS) {
     console.warn(
       `[build] Invalid BUILD_THREAD_WORKERS="${process.env.BUILD_THREAD_WORKERS}", defaulting to CPU count (${cpuCount})`,
     )
   }
-  threadWorkers = cpuCount
+  // leave one core free by default
+  threadWorkers = Math.max(1, cpuCount - 1)
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion improves system stability by default by leaving one CPU core free for other tasks, which is a sensible performance optimization.

Low
✅ Suggestions up to commit d9cc0eb
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Add Node version to cache key

Ensure the cache key incorporates the Node major version so caches don't get
mixed across Node versions, which can cause inconsistent or corrupted caches

.github/workflows/pre-release-build.yml [27-35]

 - name: Detect Node major version
-  run: echo "NODE_MAJOR=$(node -p 'process.versions.node.split(".")[0]')" >> $GITHUB_ENV
+  run: echo "NODE_MAJOR=$(node -p 'process.versions.node.split(\".\")[0]')" >> $GITHUB_ENV
 - name: Cache npm cache
   uses: actions/cache@v4
   with:
     path: ~/.npm
-    key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
+    key: ${{ runner.os }}-node${{ env.NODE_MAJOR }}-${{ hashFiles('**/package-lock.json') }}
     restore-keys: |
-      ${{ runner.os }}-node-
+      ${{ runner.os }}-node${{ env.NODE_MAJOR }}-
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the npm cache key is missing the Node.js version, which could lead to cache corruption, and aligns it with the webpack cache key that already includes it.

Medium
Prevent null stats access

Guard against a null stats object before calling hasErrors to prevent runtime
crashes on initial watch errors

build.mjs [393-410]

 const watching = compiler.watch({}, (err, stats) => {
-  const hasErrors = !!(
-    err ||
-    (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
-  )
+  const hasStats = !!stats && typeof stats.hasErrors === 'function'
+  const hasErrors = !!(err || (hasStats && stats.hasErrors()))
   const ret = callback(err, stats)
   if (process.env.BUILD_WATCH_ONCE) {
     const finalize = () =>
       watching.close((closeErr) => {
         if (closeErr) console.error('Error closing watcher:', closeErr)
-        // Exit explicitly to prevent hanging processes in CI
-        // Use non-zero exit code when errors occurred
         process.exit(hasErrors || closeErr ? 1 : 0)
       })
     if (ret && typeof ret.then === 'function') ret.then(finalize, finalize)
     else finalize()
   }
 })
Suggestion importance[1-10]: 6

__

Why: This is a valid defensive programming suggestion that prevents a potential runtime error by checking if the stats object is null before accessing its methods, improving the build script's robustness.

Low
Provide CSS sourcemap placeholders
Suggestion Impact:The commit added creation of .map files for CSS placeholders in both Chromium and Firefox development outputs, matching the suggestion to prevent 404s for CSS sourcemaps.

code diff:

-  // In development, ensure placeholder CSS files exist to avoid 404 noise
+  // In development, ensure placeholder CSS and CSS sourcemap files exist to avoid 404 noise
   if (!isProduction) {
     const chromiumCssPlaceholders = [
       path.join(chromiumOutputDir, 'popup.css'),
@@ -508,6 +530,10 @@
       if (!(await fs.pathExists(p))) {
         await fs.outputFile(p, '/* dev placeholder */\n')
       }
+      const mapPath = `${p}.map`
+      if (!(await fs.pathExists(mapPath))) {
+        await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
+      }
     }
   }
   if (isProduction) await zipFolder(chromiumOutputDir)
@@ -518,7 +544,7 @@
     [...commonFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }],
     firefoxOutputDir,
   )
-  // In development, ensure placeholder CSS files exist to avoid 404 noise
+  // In development, ensure placeholder CSS and CSS sourcemap files exist to avoid 404 noise
   if (!isProduction) {
     const firefoxCssPlaceholders = [
       path.join(firefoxOutputDir, 'popup.css'),
@@ -527,6 +553,10 @@
     for (const p of firefoxCssPlaceholders) {
       if (!(await fs.pathExists(p))) {
         await fs.outputFile(p, '/* dev placeholder */\n')
+      }
+      const mapPath = `${p}.map`
+      if (!(await fs.pathExists(mapPath))) {
+        await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
       }

Since devtool produces external source maps, also copy or generate placeholder
.map files for CSS assets in development to avoid 404s when the browser requests
*.css.map

build.mjs [496-506]

 devtool: isProduction ? false : 'cheap-module-source-map',
 ...
 await copyFiles(
   [...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
   chromiumOutputDir,
 )
-// In development, ensure placeholder CSS files exist to avoid 404 noise
+// In development, ensure placeholder CSS and CSS sourcemap files exist to avoid 404 noise
 if (!isProduction) {
   const chromiumCssPlaceholders = [
     path.join(chromiumOutputDir, 'popup.css'),
     path.join(chromiumOutputDir, 'content-script.css'),
   ]
   for (const p of chromiumCssPlaceholders) {
     if (!(await fs.pathExists(p))) {
       await fs.outputFile(p, '/* dev placeholder */\n')
     }
+    const mapPath = `${p}.map`
+    if (!(await fs.pathExists(mapPath))) {
+      await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
+    }
   }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that missing CSS sourcemaps will cause 404 errors in development, and proposes a valid fix that complements the existing placeholder logic for CSS files.

Low
Possible issue
Prevent unsafe thread-loader usage

Avoid using thread-loader together with cacheDirectory-enabled babel-loader, as
it often causes hangs or degraded performance with small/fast transforms. Gate
threading to production only, and disable threads when BUILD_WATCH_ONCE or
!isProduction. This prevents CI/dev flakiness and reduces startup overhead.

build.mjs [69-261]

-const enableThread = process.env.BUILD_THREAD === '0' ? false : true
+const enableThread =
+  process.env.BUILD_THREAD === '0'
+    ? false
+    : isProduction && !isWatchOnce // only thread in sustained prod builds
 ...
 {
   test: /\.m?jsx?$/,
   exclude: /(node_modules|bower_components)/,
   resolve: {
     fullySpecified: false,
   },
   use: [
     ...(enableThread
       ? [
           {
             loader: 'thread-loader',
             options: {
               workers: threadWorkers,
-              // Ensure one-off dev build exits quickly
-              poolTimeout: isProduction
-                ? PRODUCTION_POOL_TIMEOUT_MS
-                : isWatchOnce
-                ? 0
-                : Infinity,
+              poolTimeout: PRODUCTION_POOL_TIMEOUT_MS,
             },
           },
         ]
       : []),
     {
       loader: 'babel-loader',
       options: {
         cacheDirectory: true,
         cacheCompression: false,
         presets: ['@babel/preset-env'],
         plugins: [
           ['@babel/plugin-transform-runtime'],
           [
             '@babel/plugin-transform-react-jsx',
             {
               runtime: 'automatic',
               importSource: 'preact',
             },
           ],
         ],
       },
     },
   ],
 },
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential stability issue with using thread-loader and babel-loader's cacheDirectory together, which can cause hangs. Restricting thread-loader to production builds is a good practice that improves development stability and startup time.

Medium
✅ Suggestions up to commit 7bad746
CategorySuggestion                                                                                                                                    Impact
Possible issue
Exit non-zero on prod failures
Suggestion Impact:The commit adjusted production/build error handling to affect exit codes: it sets process.exitCode=1 when closing the compiler fails (lines 49-58) and ensures the top-level build() promise rejection exits with code 1 (lines 82-86). While it doesn’t exactly mirror the suggested finalize/hasErrors logic within compiler.run, it moves toward propagating non-zero exits on failures.

code diff:

@@ -377,7 +381,10 @@
     compiler.run((err, stats) => {
       const finishClose = () =>
         compiler.close((closeErr) => {
-          if (closeErr) console.error('Error closing compiler:', closeErr)
+          if (closeErr) {
+            console.error('Error closing compiler:', closeErr)
+            process.exitCode = 1
+          }
         })

In production, errors are logged but the process may still exit with code 0.
Propagate non-zero exit codes on build failure to avoid green CI with broken
artifacts. Mirror the dev watch-once behavior by exiting based on errors.

build.mjs [374-411]

 if (isProduction) {
-  // Ensure compiler is properly closed after production runs
   compiler.run((err, stats) => {
-    const finishClose = () =>
+    const hasErrors =
+      !!err || (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
+    const finishClose = (cb) =>
       compiler.close((closeErr) => {
         if (closeErr) console.error('Error closing compiler:', closeErr)
+        if (cb) cb(closeErr)
       })
     try {
       const ret = callback(err, stats)
+      const finalize = () =>
+        finishClose(() => {
+          if (hasErrors) process.exit(1)
+        })
       if (ret && typeof ret.then === 'function') {
-        ret.then(finishClose, finishClose)
+        ret.then(finalize, finalize)
       } else {
-        finishClose()
+        finalize()
       }
     } catch (_) {
-      finishClose()
+      finishClose(() => process.exit(1))
     }
   })
 } else {
   const watching = compiler.watch({}, (err, stats) => {
     const hasErrors = !!(
       err ||
       (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
     )
     const ret = callback(err, stats)
     if (process.env.BUILD_WATCH_ONCE) {
       const finalize = () =>
         watching.close((closeErr) => {
           if (closeErr) console.error('Error closing watcher:', closeErr)
-          // Exit explicitly to prevent hanging processes in CI
-          // Use non-zero exit code when errors occurred
           process.exit(hasErrors || closeErr ? 1 : 0)
         })
       if (ret && typeof ret.then === 'function') ret.then(finalize, finalize)
       else finalize()
     }
   })
 }
Suggestion importance[1-10]: 8

__

Why: This is a critical suggestion that correctly points out that production build failures would not propagate a non-zero exit code, potentially causing CI to pass with broken artifacts. The proposed change correctly mirrors the error handling logic from the development build, which is a significant improvement for CI reliability.

Medium
Add robust Sass fallback error
Suggestion Impact:The commit updated the Sass import logic to try 'sass-embedded' first, fall back to 'sass', and throw a clear error if both fail. This implements the suggested robust fallback error, though with a simpler error message and slightly different handling of the 'sass' import default.

code diff:

-  // Use the default export from sass-embedded; sass-loader expects the implementation object.
-  // Fallback to `sass` if sass-embedded is unavailable.
   let sassImpl
   try {
     const mod = await import('sass-embedded')
     sassImpl = mod.default || mod
-  } catch (e) {
-    const mod = await import('sass')
-    // Prefer namespace import style to avoid deprecated default import warning
-    // "import sass from 'sass'" is deprecated; use namespace instead
-    sassImpl = mod
+  } catch (e1) {
+    try {
+      const mod = await import('sass')
+      sassImpl = mod.default || mod
+    } catch (e2) {
+      throw new Error(`No Sass implementation available. Install 'sass-embedded' or 'sass'.`)
+    }
   }

If both sass-embedded and sass are unavailable, the build will crash with an
unhelpful dynamic import error. Add a clear fallback error that instructs how to
install one of the implementations. This prevents opaque failures in CI and
local envs.

build.mjs [93-102]

-const sassImpl
+let sassImpl
 try {
   const mod = await import('sass-embedded')
   sassImpl = mod.default || mod
-} catch (e) {
-  const mod = await import('sass')
-  // Prefer namespace import style to avoid deprecated default import warning
-  // "import sass from 'sass'" is deprecated; use namespace instead
-  sassImpl = mod
+} catch (e1) {
+  try {
+    const mod = await import('sass')
+    sassImpl = mod
+  } catch (e2) {
+    throw new Error(
+      '[build] No Sass implementation found. Install "sass-embedded" (preferred) or "sass". Original errors: ' +
+        (e1 && e1.message ? e1.message : String(e1)) +
+        ' | ' +
+        (e2 && e2.message ? e2.message : String(e2)),
+    )
+  }
 }
Suggestion importance[1-10]: 6

__

Why: This suggestion correctly identifies a potential unhandled error if both sass-embedded and sass imports fail, and proposes a robust solution to provide a clearer error message, which improves the build script's reliability.

Low
General
Add build config to cache key

The cache key ignores webpack config changes, risking stale caches and wrong
builds. Include a hash of build.mjs to bust cache when build settings change.
This prevents subtle CI-only build errors.

.github/workflows/pre-release-build.yml [37-45]

 - name: Cache Webpack filesystem cache
   uses: actions/cache@v4
   with:
     path: |
       .cache/webpack
       node_modules/.cache/webpack
-    key: ${{ runner.os }}-node${{ env.NODE_MAJOR }}-webpack-${{ hashFiles('**/package-lock.json') }}
+    key: ${{ runner.os }}-node${{ env.NODE_MAJOR }}-webpack-${{ hashFiles('**/package-lock.json', 'build.mjs') }}
     restore-keys: |
       ${{ runner.os }}-node${{ env.NODE_MAJOR }}-webpack-
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the Webpack cache key in the CI workflow is incomplete because it doesn't account for changes in build.mjs. Adding the file hash to the key is a valid and important improvement to prevent using a stale cache.

Medium
Incremental [*]
Avoid watcher edge-case crash
Suggestion Impact:The commit changed the watch-once condition to use isWatchOnce instead of reading the env var directly, aligning with the suggestion. However, it did not modify the hasErrors calculation. Impact is partial.

code diff:

         (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
       )
       const ret = callback(err, stats)
-      if (process.env.BUILD_WATCH_ONCE) {
+      if (isWatchOnce) {
         const finalize = (callbackFailed = false) =>
           watching.close((closeErr) => {
             if (closeErr) console.error('Error closing watcher:', closeErr)

Use the already computed isWatchOnce flag consistently and short‑circuit when
stats can be undefined to avoid TypeError on stats.hasErrors() in rare watcher
edge cases

build.mjs [393-410]

 const watching = compiler.watch({}, (err, stats) => {
-  const hasErrors = !!(
-    err ||
-    (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
-  )
+  const hasErrors =
+    !!err ||
+    !!(stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
   const ret = callback(err, stats)
-  if (process.env.BUILD_WATCH_ONCE) {
+  if (isWatchOnce) {
     const finalize = () =>
       watching.close((closeErr) => {
         if (closeErr) console.error('Error closing watcher:', closeErr)
-        // Exit explicitly to prevent hanging processes in CI
-        // Use non-zero exit code when errors occurred
         process.exit(hasErrors || closeErr ? 1 : 0)
       })
     if (ret && typeof ret.then === 'function') ret.then(finalize, finalize)
     else finalize()
   }
 })
Suggestion importance[1-10]: 6

__

Why: The suggestion improves code consistency by using the isWatchOnce variable and slightly enhances robustness by guarding against a potential TypeError if stats is null, making the watch-once logic cleaner and safer.

Low
Possible issue
Prevent truncated ZIP archives
Suggestion Impact:The commit changed the stream listener from 'close' to 'finish' and simplified finalize handling, aligning with the suggestion to resolve only after data is flushed and to call archive.finalize().

code diff:

     const onError = (err) => reject(err)
     output.on('error', onError)
     archive.on('error', onError)
-    output.on('close', resolve)
+    output.on('finish', resolve)
     archive.pipe(output)
     archive.directory(dir, false)
-    try {
-      archive.finalize()
-    } catch (err) {
-      onError(err)
-    }
+    archive.finalize()
   })

Ensure the promise resolves only after the archive stream finishes writing.
Listening to close on the output may miss buffered writes; use
output.on('finish', ...) and also wait for archive.finalize() resolution. This
prevents truncated ZIPs in CI under heavy I/O.

build.mjs [414-428]

 async function zipFolder(dir) {
   const zipPath = `${dir}.zip`
   await fs.ensureDir(path.dirname(zipPath))
   await new Promise((resolve, reject) => {
     const output = fs.createWriteStream(zipPath)
     const archive = archiver('zip', { zlib: { level: 9 } })
     const onError = (err) => reject(err)
     output.on('error', onError)
     archive.on('error', onError)
-    output.on('close', resolve)
+    // 'finish' indicates all data has been flushed to the underlying system
+    output.on('finish', resolve)
     archive.pipe(output)
     archive.directory(dir, false)
     archive.finalize().catch(onError)
   })
 }
Suggestion importance[1-10]: 8

__

Why: This is a critical bug fix; using the close event on a write stream can lead to truncated files, and the suggestion correctly proposes using the finish event to ensure all data is written before resolving the pro...

@PeterDaveHello
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
package.json (1)

64-99: Add @babel/runtime to package dependencies
Babel’s @babel/plugin-transform-runtime is enabled in build.mjs (line 180) and listed under devDependencies, but @babel/runtime is missing—this will cause runtime “module not found” errors.

--- package.json
+++ package.json
@@ "dependencies": {
+  "@babel/runtime": "^7.24.7",
 }
build.mjs (1)

173-193: @babel/plugin-transform-runtime is placed inside presets; it won’t run

The plugin configured under presets is ignored. Move it to plugins (and, if you keep it, ensure @babel/runtime is added to package.json dependencies).

               options: {
                 cacheDirectory: true,
                 cacheCompression: false,
-                presets: [
-                  '@babel/preset-env',
-                  {
-                    plugins: ['@babel/plugin-transform-runtime'],
-                  },
-                ],
-                plugins: [
+                presets: ['@babel/preset-env'],
+                plugins: [
+                  ['@babel/plugin-transform-runtime'],
                   [
                     '@babel/plugin-transform-react-jsx',
                     {
                       runtime: 'automatic',
                       importSource: 'preact',
                     },
                   ],
                 ],
               },

Follow-up (package.json):

  • Add "@babel/runtime" to dependencies if you keep the plugin, or remove the plugin here to avoid introducing a runtime dep.
🧹 Nitpick comments (3)
package.json (1)

91-97: sass + sass-embedded together: consider dropping one

build.mjs uses sass-embedded as the loader implementation. Keeping both sass and sass-embedded increases install size; unless another tool requires sass, consider removing sass to slim CI installs. If you do need both, no action needed.

.github/workflows/pre-release-build.yml (1)

35-43: Broaden the webpack cache key to include build.mjs

Including build.mjs in the cache key avoids restoring a cache that webpack will immediately invalidate due to buildDependencies. This saves time and bandwidth.

Apply:

-          key: ${{ runner.os }}-webpack-${{ hashFiles('**/package-lock.json') }}
+          key: ${{ runner.os }}-webpack-${{ hashFiles('**/package-lock.json', 'build.mjs') }}
-          restore-keys: |
-            ${{ runner.os }}-webpack-
+          restore-keys: |
+            ${{ runner.os }}-webpack-
.github/copilot-instructions.md (1)

33-35: Tiny grammar/polish

Make the performance defaults sentence crisper.

-Performance defaults: esbuild is used for JS/CSS minification; dev injects CSS via style-loader,
-prod extracts CSS via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
+Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader,
+in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3768a06 and b76041e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • .github/copilot-instructions.md (1 hunks)
  • .github/workflows/pre-release-build.yml (1 hunks)
  • build.mjs (12 hunks)
  • package.json (2 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md

[grammar] ~33-~33: There might be a mistake here.
Context: ...ation; dev injects CSS via style-loader, prod extracts CSS via MiniCssExtractPlug...

(QB_NEW_EN)

🔇 Additional comments (8)
package.json (1)

77-79: Verify esbuild-loader 4.3.0 compatibility with webpack ^5.92.1 and Node 20
package.json lists:
• webpack ^5.92.1
• esbuild ^0.25.9
• esbuild-loader ^4.3.0
Ensure your CI (running Node v20.x) installs these exact versions and completes a full webpack build without errors (for example, npm ci && npm run build), or run npm ls webpack esbuild-loader to confirm no version conflicts.

.github/workflows/pre-release-build.yml (1)

27-34: NPM cache step: LGTM

Positioning before npm ci and scoping to ~/.npm is correct for faster restores without locking node_modules.

.github/copilot-instructions.md (1)

20-32: Docs match implementation

The documented BUILD_PARALLEL, BUILD_THREAD/BUILD_THREAD_WORKERS, BUILD_CACHE_COMPRESSION, and BUILD_WATCH_ONCE flags align with build.mjs behavior. Nice.

build.mjs (5)

92-101: Switch to esbuild for JS/CSS minification: LGTM

Configuration and targets look sane; legalComments: 'none' keeps bundles lean, and CSS uses esbuildMinify via css-minimizer.


160-171: Threading defaults and poolTimeout handling are appropriate

Enabling thread-loader by default with CPU-derived workers and using poolTimeout = 0 for BUILD_WATCH_ONCE is a good balance for dev/prod.


364-375: Dev CSS placeholders are a nice touch

Avoiding 404 noise when using style-loader in development improves DX without affecting production.

Also applies to: 385-395


237-246: Font asset handling: confirm fallback behavior

woff2 is inlined, while woff/ttf are marked emit: false. That’s fine for modern Chromium/Firefox, but verify there’s no runtime path that requests woff/ttf (e.g., older Firefox ESR or custom user agents). If needed, either remove emit:false or conditionally emit only for production.

Suggested tweak if you need to emit in prod:

-        {
-          test: /\.(woff|ttf)$/,
-          type: 'asset/resource',
-          generator: {
-            emit: false,
-          },
-        },
+        {
+          test: /\.(woff|ttf)$/,
+          type: 'asset/resource',
+          generator: { emit: !isProduction ? false : true },
+        },

82-90: Filesystem cache compression control: LGTM

Env-driven compression with a safe default (disabled) aligns with the perf objective and warm-build wins.

@PeterDaveHello
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 31, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@PeterDaveHello
Copy link
Member Author

@codex review

@PeterDaveHello PeterDaveHello requested a review from Copilot August 31, 2025 07:20

This comment was marked as outdated.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Comment on lines 55 to +165
output: {
filename: '[name].js',
path: path.resolve(__dirname, outdir),
path: path.resolve(__dirname, sourceBuildDir || outdir),
},
mode: isProduction ? 'production' : 'development',
devtool: isProduction ? false : 'inline-source-map',
devtool: isProduction ? false : 'cheap-module-source-map',
cache: {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Copy emitted source maps into dev build output

Development builds switched devtool to cheap-module-source-map, which emits external *.js.map files. However the packaging step only copies the JS and CSS assets into build/chromium*/build/firefox* and never copies the generated source map files. When loading the unpacked extension in a browser the bundles will reference content-script.js.map, popup.js.map, etc., but those files are missing, so DevTools reports 404s and source maps are unusable. Either copy the .map files alongside the bundles or keep inline source maps to avoid the broken references.

Useful? React with 👍 / 👎.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (4)
build.mjs (4)

35-56: Thread worker calculation has proper bounds checking.

The logic for determining thread workers is robust with proper validation and capping to CPU count. Good defensive programming.


57-58: Consider making the production pool timeout configurable.

The hardcoded 2000ms timeout could be made configurable for different environments.

-const PRODUCTION_POOL_TIMEOUT_MS = 2000
+const PRODUCTION_POOL_TIMEOUT_MS = process.env.BUILD_POOL_TIMEOUT ? 
+  parseInt(process.env.BUILD_POOL_TIMEOUT, 10) : 2000

372-385: Add logging for skipped files in development.

When files are skipped (especially CSS placeholders), it would be helpful to log this for debugging purposes.

           if (!isProduction && isCss) return
+          console.log(`[build] Skipping missing CSS file: ${entryPoint.src} (placeholder will be created)`)

456-508: Parallel build implementation with temporary directories is well-structured.

The parallel and sequential build paths are properly implemented with appropriate cleanup of temporary directories. The error handling ensures failed builds are properly reported.

🧹 Nitpick comments (4)
package.json (1)

92-94: Consider pinning sass-embedded to match sass version.

While both sass and sass-embedded are at ^1.91.0, having separate version specs could lead to divergence. Consider using a single shared version constraint or removing the redundant sass package if sass-embedded is the primary implementation.

If sass-embedded is the primary Sass implementation being used in build.mjs, you could remove the regular sass package:

-    "sass": "^1.91.0",
     "sass-embedded": "^1.91.0",
.github/workflows/pre-release-build.yml (1)

29-35: Consider adding npm cache version to the cache key.

The npm cache key only includes the OS and package-lock hash. If npm itself is upgraded, the cache structure might change. Consider including npm version in the key.

-          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
+          key: ${{ runner.os }}-npm-v${{ steps.npm-version.outputs.version }}-${{ hashFiles('**/package-lock.json') }}

You would need to add a step to capture npm version:

      - name: Get npm version
        id: npm-version
        run: echo "version=$(npm -v)" >> $GITHUB_OUTPUT
.github/copilot-instructions.md (1)

33-34: Fix minor grammar issue in the documentation.

There's a minor grammar issue flagged by static analysis.

-Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader,
-in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
+Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader;
+in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
build.mjs (1)

20-34: Consider extracting cache compression validation to a helper function.

The cache compression validation logic is quite verbose and could be simplified.

-const cacheCompressionEnv = process.env.BUILD_CACHE_COMPRESSION
-let cacheCompressionOption
-if (cacheCompressionEnv != null) {
-  const v = String(cacheCompressionEnv).trim().toLowerCase()
-  if (v === '' || v === '0' || v === 'false' || v === 'none') {
-    cacheCompressionOption = false
-  } else if (v === 'gzip' || v === 'brotli') {
-    cacheCompressionOption = v
-  } else {
-    console.warn(
-      `[build] Unknown BUILD_CACHE_COMPRESSION="${cacheCompressionEnv}", defaulting to no compression`,
-    )
-    cacheCompressionOption = false
-  }
-}
+function parseCacheCompression(env) {
+  if (env == null) return undefined
+  const v = String(env).trim().toLowerCase()
+  if (['', '0', 'false', 'none'].includes(v)) return false
+  if (['gzip', 'brotli'].includes(v)) return v
+  console.warn(`[build] Unknown BUILD_CACHE_COMPRESSION="${env}", defaulting to no compression`)
+  return false
+}
+const cacheCompressionOption = parseCacheCompression(process.env.BUILD_CACHE_COMPRESSION)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b76041e and 78d7999.

📒 Files selected for processing (4)
  • .github/copilot-instructions.md (1 hunks)
  • .github/workflows/pre-release-build.yml (1 hunks)
  • build.mjs (12 hunks)
  • package.json (3 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md

[grammar] ~33-~33: There might be a mistake here.
Context: ...opment CSS is injected via style-loader, in production CSS is extracted via MiniC...

(QB_NEW_EN)

🔇 Additional comments (12)
package.json (3)

78-79: Ensure esbuild is actually installed before the build runs.

Adding esbuild and esbuild-loader to devDependencies is correct for the new minification pipeline.


96-97: LGTM! Build performance tooling additions.

The addition of style-loader for development builds and thread-loader for parallel processing aligns well with the performance optimization goals.


22-22: Keep @babel/runtime in dependencies
@babel/plugin-transform-runtime rewrites your code to import helpers (and regenerator runtime by default) from @babel/runtime, so it must remain in dependencies, not devDependencies.

Likely an incorrect or invalid review comment.

.github/workflows/pre-release-build.yml (2)

27-28: Node major version detection looks good.

The approach for detecting Node major version is correct and will properly differentiate caches across Node versions.


37-45: Webpack cache configuration is well-structured.

The cache paths and key structure appropriately include Node major version, which is important for native dependencies that might be compiled differently across Node versions.

.github/copilot-instructions.md (1)

22-31: Documentation of build performance options is comprehensive.

The new build performance options are well-documented with clear defaults and use cases. The BUILD_PARALLEL and BUILD_THREAD options provide good control over resource usage.

build.mjs (6)

17-18: LGTM! Parallel build control implementation.

The parallel build toggle with sensible default (on) provides good flexibility for different build environments.


82-82: Good fix for sass-embedded import issue.

The destructuring of the default export properly addresses the previous review comment about sass-loader expecting the implementation object.


129-136: Excellent choice using esbuild for minification.

Using esbuild for both JS and CSS minification should provide significant build performance improvements compared to Terser.


196-211: Thread-loader configuration is well-implemented.

The conditional inclusion of thread-loader with appropriate pool timeout settings for different scenarios (production, watch-once, development) is well thought out.


345-354: Watch-once implementation is clean and correct.

The implementation properly closes the watcher after the first build when BUILD_WATCH_ONCE is set.


417-427: Good solution for CSS 404 noise in development.

Creating placeholder CSS files in development mode is a thoughtful solution to avoid console noise from 404 errors.

Also applies to: 437-446

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (6)
build.mjs (6)

17-17: Simplify boolean logic for parallelBuild

The ternary operator with explicit boolean values is unnecessarily verbose.

-const parallelBuild = process.env.BUILD_PARALLEL === '0' ? false : true
+const parallelBuild = process.env.BUILD_PARALLEL !== '0'

35-42: Simplify CPU count fallback logic

The CPU count determination logic is overly complex and can be simplified using Math.max.

-let cpuCount = 1
-try {
-  const cpuInfo = os.cpus && os.cpus()
-  const len = Array.isArray(cpuInfo) ? cpuInfo.length : 0
-  cpuCount = Number.isInteger(len) && len > 0 ? len : 1
-} catch {
-  cpuCount = 1
-}
+const cpuCount = Math.max(1, os.cpus()?.length || 1)

130-130: Verify source map configuration change impact

The change from 'inline-source-map' to 'cheap-module-source-map' improves build performance but creates external .map files that need to be copied in development builds.

#!/bin/bash
# Check if .map files are generated and if there's any handling for them
echo "Checking for source map handling in development builds..."
rg -n "\.map" build.mjs
rg -n "source-map" build.mjs

# Check if copyFiles function handles .map files
ast-grep --pattern 'copyFiles($$$)'

506-558: Consider extracting duplicated webpack build logic

The production build contains significant code duplication between parallel and sequential builds. This pattern of wrapping runWebpack in promises is repeated multiple times.

Consider refactoring to reduce duplication:

+function createWebpackBuildPromise(isWithoutKatex, isWithoutTiktoken, minimal, tmpDir, suffix) {
+  return new Promise((resolve, reject) =>
+    runWebpack(isWithoutKatex, isWithoutTiktoken, minimal, tmpDir, async (err, stats) => {
+      if (err || stats.hasErrors()) {
+        console.error(err || stats.toString())
+        reject(err || new Error('webpack error'))
+        return
+      }
+      await finishOutput(suffix, tmpDir)
+      resolve()
+    })
+  )
+}

 async function build() {
   await deleteOldDir()
   if (isProduction && !isAnalyzing) {
     const tmpFull = `${outdir}/.tmp-full`
     const tmpMin = `${outdir}/.tmp-min`
+    
+    const buildMin = () => createWebpackBuildPromise(true, true, true, tmpMin, '-without-katex-and-tiktoken')
+    const buildFull = () => createWebpackBuildPromise(false, false, false, tmpFull, '')
+    
     if (parallelBuild) {
-      await Promise.all([
-        new Promise((resolve, reject) =>
-          runWebpack(true, true, true, tmpMin, async (err, stats) => {
-            // ... duplicated code
-          }),
-        ),
-        new Promise((resolve, reject) =>
-          runWebpack(false, false, false, tmpFull, async (err, stats) => {
-            // ... duplicated code
-          }),
-        ),
-      ])
+      await Promise.all([buildMin(), buildFull()])
     } else {
-      await new Promise((resolve, reject) =>
-        runWebpack(true, true, true, tmpMin, async (err, stats) => {
-          // ... duplicated code
-        }),
-      )
-      await new Promise((resolve, reject) =>
-        runWebpack(false, false, false, tmpFull, async (err, stats) => {
-          // ... duplicated code
-        }),
-      )
+      await buildMin()
+      await buildFull()
     }
     await fs.rm(tmpFull, { recursive: true, force: true })
     await fs.rm(tmpMin, { recursive: true, force: true })
     return
   }
   // ... rest of the function
 }

98-104: Consider using a more stable cache key

Including the full sourceBuildDir path in the cache key may cause unnecessary cache invalidation when build directories change between runs.

 const variantId = [
   isWithoutKatex ? 'no-katex' : 'with-katex',
   isWithoutTiktoken ? 'no-tiktoken' : 'with-tiktoken',
   minimal ? 'minimal' : 'full',
-  sourceBuildDir || outdir,
+  path.basename(sourceBuildDir || outdir),
   isProduction ? 'prod' : 'dev',
 ].join('|')

420-436: Critical: Build may silently skip missing artifacts

The copyFiles function now silently skips missing files when fs.pathExists returns false, which could lead to incomplete builds being published without any failure signal. While CSS placeholders in dev mode are acceptable, critical JS files should never be skipped silently.

Consider failing the build for missing critical files:

 async function copyFiles(entryPoints, targetDir) {
   if (!fs.existsSync(targetDir)) await fs.mkdir(targetDir, { recursive: true })
   await Promise.all(
     entryPoints.map(async (entryPoint) => {
       try {
         if (await fs.pathExists(entryPoint.src)) {
           await fs.copy(entryPoint.src, `${targetDir}/${entryPoint.dst}`)
         } else {
-          // Skip missing CSS in development (placeholders will be created later)
           const isCss = String(entryPoint.dst).endsWith('.css')
-          if (!isProduction || isCss) {
-            if (!isProduction && isCss) return
+          // Only skip CSS files in development (placeholders will be created)
+          if (!isProduction && isCss) {
+            console.log(`Skipping missing CSS in dev: ${entryPoint.src}`)
+            return
           }
+          // For all other files or production builds, this is a critical error
           throw new Error(`Missing build artifact: ${entryPoint.src}`)
         }
       } catch (e) {
         console.error('Copy failed:', entryPoint, e)
         throw e
       }
     }),
   )
 }
🧹 Nitpick comments (1)
build.mjs (1)

401-415: Add error handling for directory creation in zipFolder

The zipFolder function should ensure the parent directory exists before creating the zip file.

The implementation correctly uses fs.ensureDir to create parent directories, and the Promise-based error handling is comprehensive. Good implementation!

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 78d7999 and ee5c711.

📒 Files selected for processing (1)
  • build.mjs (11 hunks)
🔇 Additional comments (6)
build.mjs (6)

89-96: LGTM! Sass implementation import fixed correctly

The sass-embedded import now correctly uses the default export, which resolves the previous runtime error issue. The fallback to sass is also properly implemented.


364-399: LGTM! Proper compiler lifecycle management

The compiler is now properly closed after production runs, preventing resource leaks. The watch mode also correctly handles the BUILD_WATCH_ONCE scenario with appropriate exit codes.


467-477: LGTM! Good solution for CSS 404 noise in development

The placeholder CSS files effectively prevent 404 errors in development mode without affecting production builds.

Also applies to: 487-497


149-156: LGTM! Effective esbuild integration for minification

The migration from Terser to esbuild for both JS and CSS minification is well-implemented and should provide significant build performance improvements as reported in the PR objectives.


216-230: LGTM! Well-configured thread-loader integration

The thread-loader configuration with dynamic worker allocation based on CPU cores and appropriate pool timeouts for different build scenarios is excellent.


255-255: LGTM! Smart CSS handling for dev vs production

Using style-loader in development and MiniCssExtractPlugin in production is a best practice that speeds up development builds while maintaining proper CSS extraction for production.

Also applies to: 271-271, 286-286

@PeterDaveHello PeterDaveHello requested a review from Copilot August 31, 2025 07:57

This comment was marked as outdated.

@PeterDaveHello
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
build.mjs (3)

91-98: Stabilize cache key: avoid absolute paths in variantId to improve cache hits across environments

Using sourceBuildDir or outdir directly can include absolute paths, fragmenting cache keys between CI/local. Normalize to a relative path.

   const variantId = [
     isWithoutKatex ? 'no-katex' : 'with-katex',
     isWithoutTiktoken ? 'no-tiktoken' : 'with-tiktoken',
     minimal ? 'minimal' : 'full',
-    sourceBuildDir || outdir,
+    path.relative(process.cwd(), sourceBuildDir || outdir),
     isProduction ? 'prod' : 'dev',
   ].join('|')

376-389: Fix watch-once: await async callback before closing watcher/exiting

When BUILD_WATCH_ONCE is set, the code exits before async work in the callback (e.g., finishOutput) completes, yielding partial outputs. Await the returned promise before closing the watcher and exiting.

-    const watching = compiler.watch({}, (err, stats) => {
+    const watching = compiler.watch({}, (err, stats) => {
       const hasErrors = !!(
         err ||
         (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
       )
-      callback(err, stats)
-      if (process.env.BUILD_WATCH_ONCE) {
-        watching.close((closeErr) => {
-          if (closeErr) console.error('Error closing watcher:', closeErr)
-          // Exit explicitly to prevent hanging processes in CI
-          // Use non-zero exit code when errors occurred
-          process.exit(hasErrors || closeErr ? 1 : 0)
-        })
-      }
+      const ret = callback(err, stats)
+      const finalize = () =>
+        watching.close((closeErr) => {
+          if (closeErr) console.error('Error closing watcher:', closeErr)
+          process.exit(hasErrors || closeErr ? 1 : 0)
+        })
+      if (process.env.BUILD_WATCH_ONCE) {
+        if (ret && typeof ret.then === 'function') ret.then(finalize, finalize)
+        else finalize()
+      }
     })

123-124: Dev source maps: either copy .map files or revert to inline to avoid 404s in unpacked extension

With devtool set to cheap-module-source-map, bundles reference external *.js.map files, but finishOutput doesn’t copy them, causing broken source maps in the browser. Choose one:

Option A — copy maps:

@@
-    devtool: isProduction ? false : 'cheap-module-source-map',
+    devtool: isProduction ? false : 'cheap-module-source-map',
@@
-async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) {
+async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) {
+  const mapFiles = isProduction
+    ? []
+    : [
+        { src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' },
+        { src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
+        { src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
+        { src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
+        { src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
+      ]
@@
-  await copyFiles(
-    [...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
+  await copyFiles(
+    [...commonFiles, ...mapFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
     chromiumOutputDir,
   )
@@
-  await copyFiles(
-    [...commonFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }],
+  await copyFiles(
+    [...commonFiles, ...mapFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }],
     firefoxOutputDir,
   )

Option B — revert to inline source maps in dev:

-    devtool: isProduction ? false : 'cheap-module-source-map',
+    devtool: isProduction ? false : 'inline-source-map',

Run to confirm maps are included after applying Option A:

#!/bin/bash
rg -n "js\.map" -C2 build.mjs && echo "---" && rg -n "shared\.js\.map|content-script\.js\.map|background\.js\.map|popup\.js\.map|IndependentPanel\.js\.map" -C1

Also applies to: 438-456, 460-463, 480-483

🧹 Nitpick comments (6)
build.mjs (3)

411-411: Prefer fs.ensureDir for idempotent directory creation

Slightly simpler and race-safe.

-  if (!fs.existsSync(targetDir)) await fs.mkdir(targetDir, { recursive: true })
+  await fs.ensureDir(targetDir)

187-187: Document resolve.symlinks: false and its implications

This can affect npm/pnpm link and workspace symlinks. Add a note in docs (Build Performance or Bootstrap) about potential impact and workarounds.


499-571: Deduplicate build variant orchestration to reduce boilerplate

The Promise wrapping for the two production variants is repeated. Extract a small helper to run webpack and finishOutput to cut duplication and lower error surface.

Example:

function webpackRunPromise(args, suffix, dir) {
  return new Promise((resolve, reject) =>
    runWebpack(...args, async (err, stats) => {
      if (err || stats.hasErrors()) {
        console.error(err || stats.toString())
        reject(err || new Error('webpack error'))
        return
      }
      await finishOutput(suffix, dir)
      resolve()
    }),
  )
}

// Usage:
const buildMin = () => webpackRunPromise([true, true, true, tmpMin], '-without-katex-and-tiktoken', tmpMin)
const buildFull = () => webpackRunPromise([false, false, false, tmpFull], '', tmpFull)
.github/copilot-instructions.md (3)

33-35: Tighten grammar/punctuation for clarity

Add commas around parenthetical phrases.

-Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader;
-in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
+Performance defaults: esbuild handles JS/CSS minification; in development, CSS is injected via style-loader;
+in production, CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.

20-32: Document symlink behavior (resolve.symlinks: false) and local-link caveats

Add a short note so contributors using npm/pnpm link aren’t surprised.

Suggested addition:

  • Note: Webpack is configured with resolve.symlinks: false to speed up/normalize module resolution. If you use npm link or pnpm workspaces with symlinked deps, local changes may not be picked up as expected. Workarounds: avoid symlinked deps for this repo, or temporarily enable symlink resolution in build.mjs while developing the linked package.

36-47: Mention dev source maps location/expectations

Since dev builds emit external source maps, note that .map files are included in build/* outputs to keep DevTools happy.

Suggested addition:

  • Development: Bundles reference external source maps (.js.map). These files are copied into build/chromium/ and build/firefox*/ so DevTools can load them without 404s. If you don’t need source maps, set BUILD_SOURCEMAPS=0 (future option) or switch to inline maps locally.

Also applies to: 104-117

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ee5c711 and 4e0e3a7.

📒 Files selected for processing (4)
  • .github/copilot-instructions.md (1 hunks)
  • .github/workflows/pre-release-build.yml (1 hunks)
  • build.mjs (11 hunks)
  • package.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • .github/workflows/pre-release-build.yml
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md

[grammar] ~33-~33: There might be a mistake here.
Context: ...opment CSS is injected via style-loader; in production CSS is extracted via MiniC...

(QB_NEW_EN)

🔇 Additional comments (4)
build.mjs (4)

80-89: Correct sass-embedded default export usage — LGTM

Using the default export ensures sass-loader receives the implementation object; fallback to sass is correct.


35-55: Thread worker parsing/capping — LGTM

Positive integer validation, CPU cap, and warnings look solid. Good defaulting to cpuCount.


56-59: Thread-loader timeout constant — LGTM

Named constant resolves the magic number. Pool timeout choices make sense (2s prod, 0 for watch-once, Infinity in dev).

Also applies to: 209-224


404-407: Remove .catch on archiver.finalize() (returns void) to avoid sync TypeError and broken zips

archiver.finalize() does not return a promise; calling .catch throws synchronously and can break the zip step.

-    archive.directory(dir, false)
-    archive.finalize().catch(onError)
+    archive.directory(dir, false)
+    try {
+      archive.finalize()
+    } catch (err) {
+      onError(err)
+    }

Likely an incorrect or invalid review comment.

@PeterDaveHello PeterDaveHello requested a review from Copilot August 31, 2025 11:36

This comment was marked as outdated.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
build.mjs (3)

423-449: Avoid TOCTOU on copy; let fs.copy fail and handle ENOENT.

Removes the race between pathExists and copy while keeping dev CSS skip behavior.

 async function copyFiles(entryPoints, targetDir) {
   if (!fs.existsSync(targetDir)) await fs.mkdir(targetDir, { recursive: true })
   await Promise.all(
     entryPoints.map(async (entryPoint) => {
-      try {
-        if (await fs.pathExists(entryPoint.src)) {
-          await fs.copy(entryPoint.src, `${targetDir}/${entryPoint.dst}`)
-        } else {
-          // Skip missing CSS in development (placeholders will be created later)
-          const isCss = String(entryPoint.dst).endsWith('.css')
-          if (!isProduction || isCss) {
-            if (!isProduction && isCss) {
-              console.log(
-                `[build] Skipping missing CSS file: ${entryPoint.src} -> ${entryPoint.dst} (placeholder will be created)`,
-              )
-              return
-            }
-          }
-          throw new Error(`Missing build artifact: ${entryPoint.src}`)
-        }
-      } catch (e) {
-        console.error('Copy failed:', entryPoint, e)
-        throw e
-      }
+      try {
+        await fs.copy(entryPoint.src, `${targetDir}/${entryPoint.dst}`)
+      } catch (e) {
+        const isCss = String(entryPoint.dst).endsWith('.css')
+        if (e && e.code === 'ENOENT') {
+          if (!isProduction && isCss) {
+            console.log(
+              `[build] Skipping missing CSS file: ${entryPoint.src} -> ${entryPoint.dst} (placeholder will be created)`,
+            )
+            return
+          }
+          console.error('Missing build artifact:', entryPoint.src)
+        } else {
+          console.error('Copy failed:', entryPoint, e)
+        }
+        throw e
+      }
     }),
   )
 }

451-469: Dev source maps aren’t copied; DevTools shows 404s. Add .map copies in dev.

Since devtool is cheap-module-source-map, include emitted maps alongside bundles.

 async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) {
-  const commonFiles = [
+  const commonFiles = [
     { src: 'src/logo.png', dst: 'logo.png' },
     { src: 'src/rules.json', dst: 'rules.json' },
 
     { src: `${sourceBuildDir}/shared.js`, dst: 'shared.js' },
     { src: `${sourceBuildDir}/content-script.css`, dst: 'content-script.css' }, // shared
 
     { src: `${sourceBuildDir}/content-script.js`, dst: 'content-script.js' },
 
     { src: `${sourceBuildDir}/background.js`, dst: 'background.js' },
 
     { src: `${sourceBuildDir}/popup.js`, dst: 'popup.js' },
     { src: `${sourceBuildDir}/popup.css`, dst: 'popup.css' },
     { src: 'src/popup/index.html', dst: 'popup.html' },
 
     { src: `${sourceBuildDir}/IndependentPanel.js`, dst: 'IndependentPanel.js' },
     { src: 'src/pages/IndependentPanel/index.html', dst: 'IndependentPanel.html' },
-  ]
+    // Dev-only: copy external source maps for CSP-safe debugging
+    ...(isProduction
+      ? []
+      : [
+          { src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
+          { src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
+          { src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
+          { src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
+        ]),
+  ]

370-404: Watch-once may exit before async finishOutput completes. Await the callback.

Process can terminate early, leaving partial outputs. Chain the promise before closing watcher/exiting.

   } else {
-    const watching = compiler.watch({}, (err, stats) => {
+    const watching = compiler.watch({}, (err, stats) => {
       const hasErrors = !!(
         err ||
         (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())
       )
-      callback(err, stats)
+      const ret = callback(err, stats)
       if (process.env.BUILD_WATCH_ONCE) {
-        watching.close((closeErr) => {
-          if (closeErr) console.error('Error closing watcher:', closeErr)
-          // Exit explicitly to prevent hanging processes in CI
-          // Use non-zero exit code when errors occurred
-          process.exit(hasErrors || closeErr ? 1 : 0)
-        })
+        const finalize = () =>
+          watching.close((closeErr) => {
+            if (closeErr) console.error('Error closing watcher:', closeErr)
+            process.exit(hasErrors || closeErr ? 1 : 0)
+          })
+        if (ret && typeof ret.then === 'function') {
+          ret.then(finalize, finalize)
+        } else {
+          finalize()
+        }
       }
     })
   }
🧹 Nitpick comments (5)
.github/copilot-instructions.md (3)

32-33: Fix list indentation to satisfy markdownlint (MD005/MD007).

Align top-level and sub-bullets consistently.

- - BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing)
- - BUILD_POOL_TIMEOUT: Override thread-loader production pool timeout (ms)
-   - Default: `2000`. Increase if workers recycle too aggressively on slow machines/CI
+ - BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing)
+- BUILD_POOL_TIMEOUT: Override thread-loader production pool timeout (ms)
+  - Default: `2000`. Increase if workers recycle too aggressively on slow machines/CI

35-37: Tighten grammar and readability in “Performance defaults.”

Capitalize clauses and use commas/periods for clarity.

-Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader;
-in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
+Performance defaults: esbuild handles JS/CSS minification. In development, CSS is injected via style-loader;
+in production, CSS is extracted via MiniCssExtractPlugin. Thread-loader is enabled by default in both dev and prod.

20-37: Document source map behavior (external .map files in dev).

Dev builds use external source maps; call out that .map files are written next to bundles to avoid DevTools 404s and that production builds omit them.

 - BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing)
+- Source maps (dev): External `.map` files are emitted next to JS bundles for CSP‑safe debugging; production builds disable source maps.
build.mjs (2)

136-151: Cache invalidation: include lockfiles/package.json in filesystem cache deps.

Without these, cache may go stale when deps change.

   cache: {
     type: 'filesystem',
     name: `webpack-${variantId}`,
     version: JSON.stringify({
       THREAD: enableThread,
       WORKERS: threadWorkers,
       CACHE_COMP: cacheCompressionOption ?? false,
       PROD: isProduction,
     }),
     // default none; override via BUILD_CACHE_COMPRESSION=gzip|brotli
     compression: cacheCompressionOption ?? false,
     buildDependencies: {
-      config: [path.resolve('build.mjs')],
+      config: [
+        path.resolve('build.mjs'),
+        path.resolve('package.json'),
+        path.resolve('package-lock.json'),
+        path.resolve('pnpm-lock.yaml'),
+        path.resolve('yarn.lock'),
+      ],
     },
   },

199-205: Resolve.symlinks=false can surprise linked dev setups; document it.

Add a short comment here and a note in docs about pnpm/npm link implications and why it’s set.

Would you like me to add a brief blurb under “Build Performance Options” explaining this choice?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4e0e3a7 and 4211e11.

📒 Files selected for processing (2)
  • .github/copilot-instructions.md (1 hunks)
  • build.mjs (11 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md

[grammar] ~35-~35: There might be a mistake here.
Context: ...opment CSS is injected via style-loader; in production CSS is extracted via MiniC...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
.github/copilot-instructions.md

32-32: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1

(MD005, list-indent)


32-32: Unordered list indentation
Expected: 0; Actual: 1

(MD007, ul-indent)


33-33: Unordered list indentation
Expected: 2; Actual: 3

(MD007, ul-indent)

🔇 Additional comments (3)
build.mjs (3)

214-237: Thread-loader config looks solid.

Good defaults, CPU-aware worker cap, and poolTimeout tuning for dev/watch-once.


56-67: Good: pool timeout is named and env-overridable.

Clear default with validation; matches docs.


350-366: String-replace edits safe—no dangling syntax
Replacements only target standalone import/call lines in src/background/index.mjs, producing valid comment lines without stray commas or bindings, and the fonts import in src/content-script/styles.scss is present.

@PeterDaveHello
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

@PeterDaveHello
Copy link
Member Author

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Swish!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@PeterDaveHello
Copy link
Member Author

/gemini review

@PeterDaveHello
Copy link
Member Author

/gemini summary

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This is an excellent pull request that significantly improves the build process. The switch to esbuild, the addition of thread-loader, and the robust caching and parallelization strategies are all fantastic changes that will greatly enhance build performance and developer experience. The code is well-structured and handles many edge cases gracefully. I have a few minor suggestions to further improve code clarity and maintainability in the build script and its documentation, but overall, this is a very impressive and well-executed refactoring.

AGENTS.md Outdated
Comment on lines 35 to 37
- BUILD_RESOLVE_SYMLINKS: When set to `1`/`true`, re-enable Webpack symlink resolution for `npm link`/pnpm workspace development (default off to avoid duplicate module instances)
- Source maps (dev): Dev builds emit external `.map` files next to JS bundles for CSP-safe debugging; production builds disable source maps
- Symlinks: Webpack uses `resolve.symlinks: false` to improve performance and ensure consistent module identity; use `BUILD_RESOLVE_SYMLINKS=1` when you need to work with linked dependencies locally

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The documentation for BUILD_RESOLVE_SYMLINKS and Symlinks is a bit redundant as they describe the same feature. Consider merging them into a single point for better clarity and conciseness.

Suggested change
- BUILD_RESOLVE_SYMLINKS: When set to `1`/`true`, re-enable Webpack symlink resolution for `npm link`/pnpm workspace development (default off to avoid duplicate module instances)
- Source maps (dev): Dev builds emit external `.map` files next to JS bundles for CSP-safe debugging; production builds disable source maps
- Symlinks: Webpack uses `resolve.symlinks: false` to improve performance and ensure consistent module identity; use `BUILD_RESOLVE_SYMLINKS=1` when you need to work with linked dependencies locally
- Symlinks (`resolve.symlinks`): Default is `false` to improve performance and ensure consistent module identity. Set `BUILD_RESOLVE_SYMLINKS=1` to re-enable for `npm link`/pnpm workspace development.

build.mjs Outdated
Comment on lines 106 to 107
console.error('[build] Failed to load sass-embedded:', e1 && e1.message ? e1.message : e1)
console.error('[build] Failed to load sass:', e2 && e2.message ? e2.message : e2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error logging here could be improved for better debuggability. Instead of conditionally accessing the .message property, logging the entire error object (e1 and e2) would provide more context, including the stack trace, which is very helpful for diagnosing import failures.

Suggested change
console.error('[build] Failed to load sass-embedded:', e1 && e1.message ? e1.message : e1)
console.error('[build] Failed to load sass:', e2 && e2.message ? e2.message : e2)
console.error('[build] Failed to load sass-embedded:', e1)
console.error('[build] Failed to load sass:', e2)

Comment on lines 644 to 650
if (parallelBuild) {
await Promise.all([
createWebpackBuildPromise(true, true, true, tmpMin, '-without-katex-and-tiktoken'),
createWebpackBuildPromise(false, false, false, tmpFull, ''),
])
} else {
await createWebpackBuildPromise(true, true, true, tmpMin, '-without-katex-and-tiktoken')
await createWebpackBuildPromise(false, false, false, tmpFull, '')
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This block of code for handling parallel and sequential builds is a bit repetitive. You can refactor it to be more concise and maintainable by defining the build variants in an array and then iterating over them, which avoids duplicating the createWebpackBuildPromise calls.

      const buildConfigs = [
        [true, true, true, tmpMin, '-without-katex-and-tiktoken'],
        [false, false, false, tmpFull, ''],
      ];

      if (parallelBuild) {
        await Promise.all(buildConfigs.map((args) => createWebpackBuildPromise(...args)));
      } else {
        for (const args of buildConfigs) {
          await createWebpackBuildPromise(...args);
        }
      }

@gemini-code-assist
Copy link

Summary of Changes

This pull request significantly overhauls the build pipeline to boost performance, enhance configurability, and improve the developer experience. It achieves this by migrating to modern, faster tools like esbuild and sass-embedded, introducing intelligent parallelization and caching mechanisms, and refining build orchestration for both local development and CI environments. The changes aim to reduce build times, optimize resource utilization, and provide greater flexibility through environment variables.

Highlights

  • Faster Minification: Replaced Terser and default CSS minifiers with esbuild for significantly faster JavaScript and CSS minification in production builds.
  • Parallel Processing: Enabled thread-loader by default for Babel, dynamically determining worker count based on CPU cores, with environment variable overrides for fine-grained control.
  • Advanced Caching: Introduced configurable Webpack filesystem caching, allowing compression toggling for faster warm builds, and improved cache key generation for better invalidation across build variants.
  • Sass Tooling Update: Switched to sass-embedded for SASS processing and utilized style-loader in development for quicker CSS/SCSS compilation, while maintaining production output consistency.
  • CI/CD Acceleration: Implemented dependency caching for npm and Webpack filesystem cache in GitHub Actions workflows to speed up continuous integration builds.
  • Build Orchestration Refinement: Refactored the build script (build.mjs) to support both parallel and sequential production variant builds, improved error handling, and ensured clean exits for watch-once development builds.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • AGENTS.md
    • Added a new 'Build Performance Options' section detailing environment variables like BUILD_PARALLEL, BUILD_THREAD, BUILD_CACHE_COMPRESSION, BUILD_WATCH_ONCE, BUILD_POOL_TIMEOUT, and BUILD_RESOLVE_SYMLINKS.
    • Clarified behavior regarding source maps and symlink resolution.
  • build.mjs
    • Implemented environment variable parsing helpers (getBooleanEnv, parseCacheCompressionOption, parseThreadWorkerCount).
    • Introduced dynamic sass-embedded loading with fallback to sass and improved error logging.
    • Configured Webpack to use EsbuildPlugin for JS minification and CssMinimizerPlugin with esbuildMinify for CSS.
    • Integrated thread-loader for Babel with configurable worker counts and pool timeouts.
    • Updated Webpack cache configuration to use filesystem type with variant-specific naming and configurable compression.
    • Modified CSS loading to use style-loader in development and MiniCssExtractPlugin.loader in production.
    • Refactored runWebpack to accept sourceBuildDir and handle compiler lifecycle more robustly, including error propagation and graceful closing.
    • Enhanced zipFolder with improved error handling and promise resolution.
    • Updated copyFiles to handle missing CSS files in development by creating placeholders and to copy external source maps in dev.
    • Rewrote the build function to support parallel/sequential production builds and robust error handling for watch-once dev builds.
    • Added ensureDevCssPlaceholders function to create empty CSS and sourcemap files in dev to prevent 404s.
  • package-lock.json
    • Added new dependencies: @babel/runtime, esbuild, esbuild-loader, sass-embedded, style-loader, thread-loader, @bufbuild/protobuf, @parcel/watcher and its platform-specific variants, buffer-builder, detect-libc, get-tsconfig, has-flag, micromatch, node-addon-api, json-parse-better-errors, resolve-pkg-maps, rxjs, sync-child-process, sync-message-port, varint.
    • Updated sass to 1.91.0 and sass-loader to 16.0.5.
    • Removed terser-webpack-plugin, anymatch, binary-extensions, errno, fsevents, image-size, is-binary-path, make-dir, mime, needle, pify, sax.
    • Updated chokidar and readdirp versions.
    • Updated immutable to 5.1.3.
  • package.json
    • Added @babel/runtime, esbuild, esbuild-loader, sass-embedded, style-loader, thread-loader to devDependencies.
    • Updated sass and sass-loader versions.
    • Removed terser-webpack-plugin.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/pre-release-build.yml
Activity
  • CodeRabbit provided an initial comprehensive review with a walkthrough, suggested labels, and potential reviewers, noting a docstring coverage warning.
  • Qodo offered a reviewer guide, identified potential issues (e.g., runWebpack signature, watch exit logic, cache key specificity), and provided code suggestions, many of which were addressed.
  • Multiple automated reviews were triggered by PeterDaveHello using @codex and /gemini, often resulting in 'Didn't find any major issues.' or specific suggestions.
  • Suggestions from Copilot included lazy loading sass-embedded, validating threadWorkers input, naming PRODUCTION_POOL_TIMEOUT_MS, using fs.copy with error handling, simplifying boolean logic, clarifying cpuCount fallback, normalizing sourceBuildDir for cache keys, documenting symlinks: false, simplifying copyFiles conditions, and improving error messages.
  • Suggestions from Gemini/Codex included refactoring build function duplication, handling missing build artifacts, awaiting watch-once callbacks, rejecting build promises on copy failures, avoiding invalid characters in cache names, propagating setup failures, keeping dev watch process alive on initial errors, unifying boolean parsing, simplifying zipFolder logic, and improving error logging for Sass.
  • PeterDaveHello responded to CodeRabbit's legalComments suggestion with 'Wrong scope' and to buildDependencies suggestion with 'No need', indicating specific design decisions.
  • Many suggestions regarding error handling, promise resolution, environment variable parsing, Sass loading, and CI cache keys were incorporated into the pull request.

@PeterDaveHello
Copy link
Member Author

@codex review

@PeterDaveHello
Copy link
Member Author

/gemini review

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
build.mjs (1)

191-199: The maintainer (PeterDaveHello) has already determined that license preservation concerns are out of scope for this browser extension project. While documenting the build policy rationale is technically sound practice, it is not required given the project's stated scope.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9e5f428 and 00b2a1f.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • .github/workflows/pre-release-build.yml
  • AGENTS.md
  • build.mjs
  • package.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • .github/workflows/pre-release-build.yml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:165-171
Timestamp: 2025-09-14T18:01:17.933Z
Learning: In the ChatGPTBox project, the build.mjs webpack cache buildDependencies configuration should only track npm lockfiles (package-lock.json) and not include other package managers' lockfiles like pnpm-lock.yaml, yarn.lock, or bun.lockb, as confirmed by the maintainer PeterDaveHello.
📚 Learning: 2025-09-14T18:01:17.933Z
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:165-171
Timestamp: 2025-09-14T18:01:17.933Z
Learning: In the ChatGPTBox project, the build.mjs webpack cache buildDependencies configuration should only track npm lockfiles (package-lock.json) and not include other package managers' lockfiles like pnpm-lock.yaml, yarn.lock, or bun.lockb, as confirmed by the maintainer PeterDaveHello.

Applied to files:

  • build.mjs
📚 Learning: 2025-09-06T17:46:04.304Z
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.304Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.

Applied to files:

  • build.mjs
🔇 Additional comments (10)
build.mjs (10)

17-91: Well-structured environment variable parsing.

The environment helpers correctly handle various string representations and provide sensible defaults. The validation logic in parseThreadWorkerCount ensures worker counts are capped to available CPU cores, and the fallback handling is robust.


93-114: LGTM: Sass implementation loading with proper fallback.

The caching pattern ensures the dynamic import only executes once per build process, and the fallback from sass-embedded to sass handles environments where the preferred implementation isn't available. Error logging is helpful for debugging.


120-146: Good: Per-variant caching with Windows-safe separator.

The variant identifier correctly incorporates all dimensions that affect build outputs (Katex, Tiktoken, minimal, directory, environment) and uses __ as a cross-platform safe separator. The sourceBuildDir parameter enables proper per-variant output directory resolution.


147-340: Webpack configuration updates look solid.

The per-variant output paths, filesystem caching, and CSS loader switching (style-loader in dev, extraction in prod) are well-implemented. The thread-loader integration properly adjusts pool timeouts for different build modes, and the Babel configuration enables caching for faster rebuilds.


413-464: Excellent error handling and lifecycle management.

The production and watch paths correctly handle both synchronous and asynchronous callback errors. The Promise.resolve().then() normalization ensures synchronous throws are caught, and the callbackFailed tracking ensures exit codes reflect packaging failures, not just webpack compilation errors. The compiler close logic is properly sequenced.


466-505: Robust ZIP archive creation with proper stream handling.

The implementation correctly handles both fatal errors and non-fatal warnings, uses the close event for completion (ensuring file descriptors are flushed), and prevents double-settlement with the settled flag. The finalize() call is synchronous and correctly starts the archive process with completion tracked via events.


507-530: Improved error handling with appropriate CSS placeholder logic.

The copy logic correctly distinguishes between recoverable missing files (CSS in dev, where placeholders will be created) and critical missing artifacts (production or non-CSS). The error logging provides clear diagnostics, and the use of typeof check before .endsWith() prevents undefined errors.


532-546: Clean solution for dev CSS placeholder generation.

The function correctly creates placeholder CSS and sourcemap files only in development builds, preventing 404 errors when style-loader injects CSS at runtime instead of emitting files. The implementation uses proper existence checks and generates valid placeholder content.


548-611: Comprehensive output preparation with proper dev support.

The updated finishOutput correctly resolves artifacts from sourceBuildDir, includes external source maps in dev builds for CSP-safe debugging, and dynamically derives the CSS placeholder list from the common files array. The use of Set for deduplication is appropriate.


613-695: Excellent build orchestration with comprehensive error handling.

The refactored build() function properly handles:

  • Parallel vs. sequential production builds with Promise.allSettled
  • Early setup failures (dynamic import rejections) via line 637 and 688
  • Packaging failures that reject the build promise
  • Dev watch mode that stays alive on compilation errors but fails on packaging errors
  • Proper cleanup of temporary build directories in finally blocks
  • Top-level fatal error handling with exit code 1

This addresses multiple critical past review concerns about hanging builds, silent failures, and improper exit codes.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This is an excellent pull request that significantly improves the build pipeline's performance and reliability. The switch to esbuild for minification, the introduction of parallel builds, and the robust handling of async operations and process exits are all fantastic improvements. The code is well-structured and the new environment variables are well-documented. I have one suggestion to further improve the robustness of environment variable parsing.

Comment on lines 18 to 29
function getBooleanEnv(val, defaultValue) {
if (val == null) return defaultValue
const s = String(val).trim().toLowerCase()
if (
s === '' ||
s === '0' ||
s === 'false' ||
s === 'no' ||
s === 'off' ||
s === 'null' ||
s === 'undefined'
) {
return false
}
if (s === '1' || s === 'true' || s === 'yes' || s === 'on') {
return true
}
return defaultValue
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of getBooleanEnv silently falls back to the default value for any unrecognized string. This could hide typos or misconfigurations (e.g., BUILD_PARALLEL=fals would be treated as true if that's the default). To improve robustness and provide better feedback to developers, I suggest adding a warning for unknown values, similar to how parseCacheCompressionOption and parseThreadWorkerCount handle them. This makes the behavior more predictable and helps catch configuration errors early. I've also removed null and undefined as explicit falsy string values, as they are more likely to indicate a configuration issue and should also trigger a warning.

function getBooleanEnv(val, defaultValue) {
  if (val == null) return defaultValue
  const s = String(val).trim().toLowerCase()
  if (s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off') {
    return false
  }
  if (s === '1' || s === 'true' || s === 'yes' || s === 'on') {
    return true
  }
  console.warn(`[build] Unknown boolean env value "${val}", defaulting to ${defaultValue}`)
  return defaultValue
}

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 00b2a1f671

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 21 to 23
"dependencies": {
"@babel/runtime": "^7.24.7",
"@mozilla/readability": "^0.6.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Sync lockfile for new @babel/runtime dependency

package.json now declares @babel/runtime as a direct dependency, but the root dependency list in package-lock.json still omits it (see the top-level packages[""] block around lines 5‑39). With this mismatch, npm ci will error out because the lockfile is stale, preventing the build/lint steps from running in CI or locally.

Useful? React with 👍 / 👎.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
build.mjs (1)

18-36: Consider warning on unrecognized boolean values.

The function silently returns defaultValue for unrecognized strings (e.g., 'fals', 'maybe'), which could hide typos. Similar functions like parseCacheCompressionOption (line 46) and parseThreadWorkerCount (line 70) warn on invalid input.

Additionally, lines 27-28 check for the literal strings 'null' and 'undefined', which seems overly defensive—users are unlikely to set BUILD_PARALLEL='null' intentionally.

🔎 Suggested improvement
 function getBooleanEnv(val, defaultValue) {
   if (val == null) return defaultValue
   const s = String(val).trim().toLowerCase()
   if (
     s === '' ||
     s === '0' ||
     s === 'false' ||
     s === 'no' ||
-    s === 'off' ||
-    s === 'null' ||
-    s === 'undefined'
+    s === 'off'
   ) {
     return false
   }
   if (s === '1' || s === 'true' || s === 'yes' || s === 'on') {
     return true
   }
+  console.warn(`[build] Unknown boolean env value "${val}", defaulting to ${defaultValue}`)
   return defaultValue
 }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00b2a1f and 65e8ca6.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • .github/workflows/pre-release-build.yml
  • AGENTS.md
  • build.mjs
  • package.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/pre-release-build.yml
  • package.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:165-171
Timestamp: 2025-09-14T18:01:17.933Z
Learning: In the ChatGPTBox project, the build.mjs webpack cache buildDependencies configuration should only track npm lockfiles (package-lock.json) and not include other package managers' lockfiles like pnpm-lock.yaml, yarn.lock, or bun.lockb, as confirmed by the maintainer PeterDaveHello.
📚 Learning: 2025-09-14T18:01:17.933Z
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:165-171
Timestamp: 2025-09-14T18:01:17.933Z
Learning: In the ChatGPTBox project, the build.mjs webpack cache buildDependencies configuration should only track npm lockfiles (package-lock.json) and not include other package managers' lockfiles like pnpm-lock.yaml, yarn.lock, or bun.lockb, as confirmed by the maintainer PeterDaveHello.

Applied to files:

  • build.mjs
📚 Learning: 2025-09-06T17:46:04.304Z
Learnt from: PeterDaveHello
Repo: ChatGPTBox-dev/chatGPTBox PR: 886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.304Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.

Applied to files:

  • build.mjs
🔇 Additional comments (6)
AGENTS.md (1)

20-38: Documentation is accurate and well-structured.

The Build Performance Options section clearly documents all environment variables and their defaults, which align with the implementation in build.mjs. The documentation is helpful for developers configuring builds.

build.mjs (5)

93-114: LGTM! Sass implementation loading is well-designed.

The caching strategy via sassImplPromise ensures the dynamic import only happens once per process, and the fallback from sass-embedded to sass with comprehensive error logging is robust.


413-463: LGTM! Callback error handling is robust.

Both production and watch modes now correctly:

  • Catch synchronous errors from callbacks (line 438)
  • Handle async callback rejections (lines 430-434, 457-460)
  • Propagate callback failures to exit codes (lines 424, 454)

This ensures build failures surface immediately rather than hanging or silently succeeding.


532-546: LGTM! Placeholder creation is now parameterized.

The function now accepts cssFiles as a parameter rather than hardcoding filenames, addressing previous maintainability concerns. CSS placeholders prevent 404 errors when using style-loader in development.


640-662: LGTM! Production build orchestration is robust.

The parallel build path uses Promise.allSettled to wait for all variants and reports the first failure, while the sequential path awaits each build individually. The finally block ensures temporary directories are cleaned up even when builds fail.


664-690: LGTM! Development build error handling balances resilience and fail-fast.

Normal watch mode logs webpack errors but stays alive for auto-rebuild (line 672), while watch-once mode exits non-zero immediately (line 671). Packaging failures (finishOutput) always reject (line 680) to prevent silent success with incomplete artifacts.

@PeterDaveHello
Copy link
Member Author

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Hooray!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Adopt esbuild minification, enable thread-loader by default,
and support parallel production builds while keeping cache
compression configurable.
Consolidate boolean env parsing, cache the Sass
implementation per process, and tune worker defaults for
more predictable build behavior.
Propagate webpack build failures via explicit exit codes
so production builds fail loudly.
Derive dev CSS placeholders from the computed output list
to avoid hardcoded filenames.
Document BUILD_RESOLVE_SYMLINKS and cache compression
behavior, and tighten related parsing in the build script.
Ensure watch-once packaging failures exit non-zero and
simplify error logging.
Key CI webpack cache by full Node version for safety.
@PeterDaveHello
Copy link
Member Author

@codex review

@PeterDaveHello
Copy link
Member Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces substantial build performance optimizations and configuration flexibility. Key changes include migrating from TerserPlugin to EsbuildPlugin for faster JavaScript minification, integrating thread-loader for parallel Babel processing, and implementing webpack's filesystem cache with variant-specific naming and configurable compression. The build process now supports parallel execution of production variants and offers new environment variables (BUILD_PARALLEL, BUILD_THREAD, BUILD_CACHE_COMPRESSION, BUILD_WATCH_ONCE, BUILD_POOL_TIMEOUT, BUILD_RESOLVE_SYMLINKS) for fine-grained control over build behavior. Additionally, the CSS loading strategy has been updated to use style-loader in development and MiniCssExtractPlugin in production, and Sass implementation is now dynamically resolved. The build.mjs file was refactored to include helper functions for environment variable parsing and robust error handling for webpack runs and file operations, including creating placeholder CSS and sourcemap files in development to prevent 404 errors. Dependency updates in package.json and package-lock.json reflect these changes, including the addition of new build tools and removal of several unused packages. A minor review comment suggested a stylistic change to use != null for null/undefined checks for consistency.

}
function parseThreadWorkerCount(envValue, cpuCount) {
const maxWorkers = Math.max(1, cpuCount)
if (envValue !== undefined && envValue !== null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other helper functions in this file like getBooleanEnv and parseCacheCompressionOption, you can simplify this check. Using != null is a more concise way to check for both undefined and null.

Suggested change
if (envValue !== undefined && envValue !== null) {
if (envValue != null) {

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

const maxWorkers = Math.max(1, cpuCount)
if (envValue !== undefined && envValue !== null) {
const rawStr = String(envValue).trim()
if (/^[1-9]\d*$/.test(rawStr)) {
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern /^[1-9]\d*$/ will reject the value "0" as an invalid thread worker count. However, setting thread workers to 0 might be a valid scenario to disable threading entirely (which could align with BUILD_THREAD=0). Consider whether BUILD_THREAD_WORKERS=0 should be explicitly supported or if users should use BUILD_THREAD=0 instead. If the latter, the documentation should clarify that BUILD_THREAD_WORKERS must be >= 1.

Copilot uses AI. Check for mistakes.
Comment on lines +560 to 571
// Dev-only: copy external source maps for CSP-safe debugging
...(isProduction
? []
: [
{ src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' },
{ src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
{ src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
{ src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
{ src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
]),
]

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In development mode, source map files (.js.map) are attempted to be copied from sourceBuildDir but if they don't exist (for example, the first time the build runs or if webpack hasn't generated them yet), the copyFiles function will throw an error and fail the build. The copyFiles function only gracefully handles missing CSS files, not missing .map files. Consider extending the ENOENT handling to also skip missing .map files in development mode, similar to how CSS files are handled.

Suggested change
// Dev-only: copy external source maps for CSP-safe debugging
...(isProduction
? []
: [
{ src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' },
{ src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
{ src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
{ src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
{ src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
]),
]
]
// Dev-only: conditionally copy external source maps for CSP-safe debugging
if (!isProduction) {
const devSourceMapFiles = [
{ src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' },
{ src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' },
{ src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' },
{ src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' },
{ src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' },
]
for (const file of devSourceMapFiles) {
// In development, source maps may not yet exist (e.g., first build); skip missing ones.
if (await fs.pathExists(file.src)) {
commonFiles.push(file)
}
}
}

Copilot uses AI. Check for mistakes.
},
)
// runWebpack is async; catch early rejections (e.g., failed dynamic imports)
if (ret && typeof ret.then === 'function') ret.catch(reject)
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing await. The value 'ret' is always a promise.

Copilot uses AI. Check for mistakes.
}
})
// Early setup failures (e.g., dynamic imports) should fail fast
if (ret && typeof ret.then === 'function') ret.catch(reject)
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing await. The value 'ret' is always a promise.

Copilot uses AI. Check for mistakes.
return false
}
const cacheCompressionOption = parseCacheCompressionOption(process.env.BUILD_CACHE_COMPRESSION)
let cpuCount = 1
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial value of cpuCount is unused, since it is always overwritten.

Suggested change
let cpuCount = 1
let cpuCount

Copilot uses AI. Check for mistakes.
@PeterDaveHello PeterDaveHello merged commit 9537643 into ChatGPTBox-dev:master Jan 6, 2026
8 checks passed
@PeterDaveHello PeterDaveHello deleted the BuildOptimization branch January 6, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant